Allow user to set an annotation that will specify an existing PVC to … #4221
Conversation
| Namespace: postgresCluster.GetNamespace(), | ||
| }, | ||
| } | ||
| err := errors.WithStack(r.Client.Get(ctx, |
There was a problem hiding this comment.
🤔 If we check when generating the job spec, can we assume it exists here for generating the configmap?
There was a problem hiding this comment.
So, as of right now, if the annotation is in place but the PVC doesn't exist, we still create the backup job, we just don't mount the PVC (and we create a warning event). My thought behind this is that having backups is more important that having logs from backups, so cancelling the backup job just because it can't write the logs seems like overkill. Open to arguments to the contrary though. This does make me wonder though if an event is enough of an alarm to users that something is wrong. Maybe we should log an error too? 🤔 Either way, the backup will still be created. If we don't check for the PVC here where we reconcile the config, the backup job will attempt to write the logs to a directory that doesn't exist. I can't remember if it will fail or not (I'm guessing it won't fail for the same reason as earlier)... If it doesn't fail I guess we could remove the check for the PVC if we want... 🤔
There was a problem hiding this comment.
I do like to skip an API call if I can -- like, I wish we called the func to generate the jobspec before we generated the configmap, because then we could just pass the jobspec in and if we had the volume on the spec that matched the annotation on the cluster, we could assume the PVC was found.
But because we use the configmap for non-job stuff, that doesn't work. I guess we cold log into the ether if the volume wasn't there and skip a step -- but maybe another event will get through to the user.
(I also don't know the best way to get through to the user for something that's a warning but not an error.)
| if logPath != "" { | ||
| global.Set("log-path", logPath) | ||
| } else { | ||
| global.Set("log-level-file", "off") |
There was a problem hiding this comment.
do we just copy the user's options on lines 549-551?
|
|
||
| func TestAddVolumeAndMountsToPod(t *testing.T) { | ||
| pod := &corev1.PodSpec{ | ||
| Containers: []corev1.Container{ |
There was a problem hiding this comment.
🔧 Might be worth throwing in 2+ initContainers to show that this func adds to those containers as well
…be mounted to cloud backup jobs so that the backup logs can be persisted.
dsessler7
force-pushed
the
backup-logs-to-pvc
branch
from
f7a8955 to
b407f60
Compare
…be mounted to cloud backup jobs so that the backup logs can be persisted.
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
PGO-2578
Currently, the logs from backups that are being made to cloud repos are not persisted.
What is the new behavior (if this is a feature change)?
A user can now set an annotation, the value of which can point to a PVC that the cloud backups will use to persist logs to.
Other Information: