-
Notifications
You must be signed in to change notification settings - Fork 2
feat: support idling messages from core #245
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
base: main
Are you sure you want to change the base?
Conversation
d5da4ad to
862b4f7
Compare
bbc3e65 to
07f40b7
Compare
5a506d3 to
35c7e11
Compare
35c7e11 to
1f48010
Compare
1f48010 to
0e005ec
Compare
0e005ec to
7318858
Compare
7318858 to
5e5ba81
Compare
f9e6da8 to
4bd3230
Compare
4bd3230 to
7e433a1
Compare
7e433a1 to
0aaeb4d
Compare
a7f31b0 to
3bd59b2
Compare
3bd59b2 to
44f157c
Compare
44f157c to
b543e38
Compare
| prevReplicas, err := strconv.ParseInt(deployment.Annotations["service.lagoon.sh/replicas"], 10, 32) | ||
| if err != nil { | ||
| return err | ||
| } |
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 replicas was set to 0 manually (eg, via kubectl, bypassing this new feature), will this always fail/return an error? Should we allow it to succeed and always default to 1 replica instead?
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.
Yeah, defaulting to 1 should be fine. There are some overrides in the build-deploy-tool that can change the default replicas to 2 if they are enabled, but if someone is scaling services to 0 outside of aergia or other automated systems, going to 1 is an OK compromise. The next time the user would deploy, if any of the things that would result in 2 replicas being set, they would just get reapplied then.
| opLog.Info(fmt.Sprintf("deployment %s", deployment.Name)) | ||
| opLog.Info(fmt.Sprintf(`{"replicas":%d}`, *deployment.Spec.Replicas)) | ||
| // this would be nice to be a lagoon label :) | ||
| if val, ok := deployment.Labels["idling.amazee.io/idled"]; ok { |
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.
Why is this label required? It's not checked here or modified in EnvironmentServiceState. It would be nice if managing services wasn't tied to having aergia installed.
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.
Rather than re-creating all the logic for handling idling and unidling in the remote-controller, this does unfortunately mean aergia is a requirement, with a caveat.
Eventually, aergia will use lagoon based labels and be installed as part of lagoon-remote by default, however the ingress listener and automatic idling would be disabled. Effectively it would just handle manual idling and unidling messages from the API.
| pID, _ := strconv.Atoi(namespace.Labels["lagoon.sh/projectId"]) | ||
| projectID := helpers.UintPtr(uint(pID)) | ||
| idling := Idled{ | ||
| Idled: idled, |
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.
In this codepath, is it possible to know if this is auto-idled vs force-idled vs force-scaled and send back something closer to an enum? I think it would be useful if the Lagoon API/UI can say [for example, not suggesting renaming idling in this service]:
This environment is Active | Inactive (will resume automatically) | Disabled (must be manually resumed).
That could be a follow up if we don't commit to a boolean value for now.
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 is interesting. I hadn't thought about that as a function, mostly because people have only requested seeing the idled state.
Wouldn't be much to refactor to send the state if its easy enough to determine from the state.
Checklist
Initial support for idling control from the API.
This can be merged and released before support in the API is added, it will just sit dormant.