Adjust cloud backup volume behavior such that additional volumes take…#4294
Conversation
| if logVolume := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolume != "" { | ||
| util.AddCloudLogVolumeToPod(&jobSpec.Template.Spec, logVolume) | ||
| var collisionFound bool | ||
| if jobs != nil && jobs.Volumes != nil && len(jobs.Volumes.Additional) > 0 { |
There was a problem hiding this comment.
@cbandy I feel like you've been leaning towards "skip the length check, just range" (ranging over an empty list is the same as skipping the logic). Are there any cases where you wouldn't do that?
There was a problem hiding this comment.
It probably makes sense to remove it in this scenario because all we are doing inside the if block is ranging over jobs.Volumes.Additional and if it is empty that is a zero cost action.
In other places in the code (like just below this block where we mount the additional volumes), it might make sense to keep the length check just to save us the function call (util.AddAdditionalVolumesAndMounts) when it's empty.
| // TODO: I know it should be caught by CEL validation, but is it worthwhile to also | ||
| // check that Log.Path ~= "/volumes/" + existingAdditionalVolume.name here?? |
There was a problem hiding this comment.
That's a good question. I'm usually very defensive ("before we pass the info, let's make sure it's correct -- ok, now that we've passed the info, let's check it again"), but here I feel like it really could go either way, and I'm leaning towards not running the check here:
- if we change the CEL rules ("you can now attach a tmp and log there"), then we change the admission validation rules and don't need to remember all the places where the value is being used.
- if someone gets around the CEL validation, that's good information for us on our validation. (Although one way they can do that is always: just remove the CEL lines before installing the CRD.)
There was a problem hiding this comment.
Agreed. Maybe I'll just leave the TODO for now
… precedence when there is a naming collision with the pgbackrest-cloud-log-volume annotation. If annotation is present and no log path is given via the spec, use the log path that the annotation logic yields. Add/adjust tests appropriately.
dsessler7
force-pushed
the
cloud-backup-logging-transition
branch
from
7cfbdbe to
60f8c40
Compare
2 participants
… precedence when there is a naming collision with the pgbackrest-cloud-log-volume annotation. If annotation is present and no log path is given via the spec, use the log path that the annotation logic yields. Add/adjust tests appropriately.
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
Currently, if the cloud backup log volume annotation is present and its value is the same as the name of an additional volume, we attempt to mount both which results in the job not rolling out due to duplicate volume mounts.
What is the new behavior (if this is a feature change)?
Now, if the cloud backup log volume annotation is present and its value is the same as the name of an additional volume, we will not mount the volume specified by the annotation, we will only mount the additional volume. If log.path is not set in the spec for backup jobs and the annotation is present and a cloud backup job is created, it will use the log path determined by the annotation logic.
Other Information: