-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement zoneTracker for coordinating downscales across statefulsets using a ConfigMap for the prepare_downscale admission webhook #107
Conversation
… using a file in object storage for the prepare_downscale admission webhook Signed-off-by: JordanRushing <[email protected]>
…itial zone file if not present Signed-off-by: JordanRushing <[email protected]>
…storage; update zoneTracker tests; run `go mod vendor` & `go mod tidy` Signed-off-by: JordanRushing <[email protected]>
Signed-off-by: JordanRushing <[email protected]>
…on; update tests; go mod vendor & go mod tidy Signed-off-by: JordanRushing <[email protected]> Update mockBucket instantiation in test; deny admission requests when failing to bootstrap the s3 bucket client Signed-off-by: JordanRushing <[email protected]> Clean up s3 config struct Signed-off-by: JordanRushing <[email protected]> Change s3 config creation semantics for zoneTracker Signed-off-by: JordanRushing <[email protected]> Add support for gcs and azure object storage clients; go mod vendor & go mod tidy Signed-off-by: JordanRushing <[email protected]>
Signed-off-by: JordanRushing <[email protected]>
Signed-off-by: JordanRushing <[email protected]>
Signed-off-by: JordanRushing <[email protected]>
…ones; fix deadlock in lastDownscaled Signed-off-by: JordanRushing <[email protected]>
Signed-off-by: JordanRushing <[email protected]>
…g; update comment on loadZones to indicate unique sts name assumption for keys
Signed-off-by: JordanRushing <[email protected]>
Signed-off-by: JordanRushing <[email protected]>
…e bucket code Signed-off-by: JordanRushing <[email protected]>
Signed-off-by: JordanRushing <[email protected]>
… storage dependency Signed-off-by: JordanRushing <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cmd/rollout-operator/main.go
Outdated
@@ -87,6 +94,12 @@ func (cfg config) validate() error { | |||
if (cfg.kubeAPIURL == "") != (cfg.kubeConfigFile == "") { | |||
return errors.New("either configure both Kubernetes API URL and config file or none of them") | |||
} | |||
if cfg.useZoneTracker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could combine these if statements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the full background context here, but tried to help take a look.
pkg/admission/prep_downscale.go
Outdated
level.Warn(logger).Log("msg", msg, "err", err) | ||
return deny(msg) | ||
} | ||
// If using zoneTracker instead of downscale annotations, check the zone file for recent downscaling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If using zoneTracker instead of downscale annotations, check the zone file for recent downscaling. | |
// If using zoneTracker instead of downscale annotations, check the zone ConfigMap for recent downscaling. |
There are other instances of "zone file" elsewhere as well.
pkg/admission/prep_downscale.go
Outdated
} | ||
// If using zoneTracker instead of downscale annotations, check the zone file for recent downscaling. | ||
// Otherwise, check the downscale annotations. | ||
if zt != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: If we are doing the same logical operation with two different underlying implementations that seems like a good opportunity to use an interface and have a single function that decides which implementation will be used. I think that would make the logic here easier to follow, since currently zt != nil
reads to me like "are we zone tracking?", but even when zt == nil
zones are being tracked through the statefulset annotations since each zone is represented by a respective statefulset.
pkg/admission/zone_tracker_test.go
Outdated
} | ||
} | ||
|
||
func TestLoadZonesEmptyBucket(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func TestLoadZonesEmptyBucket(t *testing.T) { | |
func TestLoadZonesEmptyConfigMap(t *testing.T) { |
pkg/admission/zone_tracker.go
Outdated
return nil, nil | ||
} | ||
|
||
// If the zone file does not exist, populate it with the current time for each zone and upload it to the bucket. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If the zone file does not exist, populate it with the current time for each zone and upload it to the bucket. | |
// If the zone file does not exist, populate it with the current time for each zone and upload it to the ConfigMap. |
go.mod
Outdated
k8s.io/api v0.28.3 | ||
k8s.io/apimachinery v0.28.3 | ||
k8s.io/client-go v0.28.3 | ||
) | ||
|
||
require ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit odd another require
was added, deleting this block and rerunning go mod tidy
might fix it up.
func (zt *zoneTracker) lastDownscaled(zone string) (string, error) { | ||
zoneInfo, ok := zt.zones[zone] | ||
if !ok { | ||
return "", fmt.Errorf("zone %s not found", zone) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing the ok
back in the return rather than error
seems more direct.
} | ||
|
||
lastDownscaleStr, err := zt.lastDownscaled(sts.Name) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the current time be assumed in this case instead since that is what the initialization does? (Genuine question, not leading, having a hard time following some of the cases here).
pkg/admission/zone_tracker.go
Outdated
return nil, err | ||
} | ||
|
||
// Populate initial zones after the configmap is created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels unnecessary. The above Create
I can see to allow Update
, but if we were going to create anyway you could create with the initialized zones (or leave it empty and assume missing zones as current time?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be wrong here because at some point you do need to write a first value to have the confidence you've waited a certain amount of time.
…ile; run `go mod tidy` & `go mod vendor`; simplify zoneTracker config validation conditional Signed-off-by: JordanRushing <[email protected]>
… add tests for new functions post-refactor Signed-off-by: JordanRushing <[email protected]>
Thanks so much for the review, @andyasp -- I've pushed a commit here and to another branch to implement your suggestions and feedback: Once I'm finished iterating on the new branch, I'll merge the changes into this PR to finish the review so that we can merge and cut a new tag for the rollout operator. |
Signed-off-by: JordanRushing <[email protected]>
Signed-off-by: JordanRushing <[email protected]>
…e-configmap-refactor Prep downscale last downscale configmap refactor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit but otherwise LGTM
pkg/admission/prep_downscale.go
Outdated
client := &http.Client{ | ||
Timeout: 5 * time.Second, | ||
} | ||
|
||
var zt *zoneTracker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) You don't need this variable here, it can stay inside the if useZoneTracker
branch.
Signed-off-by: JordanRushing <[email protected]>
… encounters a new zone instead of erroring Signed-off-by: JordanRushing <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, looks great!
Signed-off-by: JordanRushing <[email protected]>
* Add CHANGELOG.md entry for #107 Signed-off-by: JordanRushing <[email protected]> * Update CHANGELOG.md --------- Signed-off-by: JordanRushing <[email protected]> Co-authored-by: Andy Asp <[email protected]>
Can we include this in the read.me as well? Thanks! |
Replacing #96 with this PR, changing the implementation to use a ConfigMap instead of a file in object storage.
The previous PR includes context for the reasoning behind implementing the zoneTracker and ultimately settling on a ConfigMap as the mechanism to store the relevant zone metadata.