Add set instancegroup command
#8531
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @gabrieljackson! |
|
Hi @gabrieljackson. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gabrieljackson The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I was pretty sure I had the CLA stuff all worked out, but I guess not. I will look into why the check failed. |
gabrieljackson
force-pushed
the
set-instancegroups-cmd
branch
from
4d1b8bc
to
afdfa44
Compare
k8s-ci-robot
added
cncf-cla: yes
gabrieljackson
force-pushed
the
set-instancegroups-cmd
branch
3 times, most recently
from
3deedab
to
d2efb62
Compare
This change adds a new command and functionality for updating instance group configuration via command line arguments. This behavior mimics the `set cluster` command.
gabrieljackson
force-pushed
the
set-instancegroups-cmd
branch
from
d2efb62
to
0554476
Compare
k8s-ci-robot
added
size/XL
|
/assign @rdrgmnzs |
|
|
||
| ig.Spec.MaxSize = &i32 | ||
| return nil | ||
| }, |
There was a problem hiding this comment.
I'd think spec.machineType would be important. Zones? MaxPrice?
There was a problem hiding this comment.
Agreed. As noted in my original PR description, I started with a small list of config fields with the intention to add more if the proposed implementation was acceptable as a first-pass for the command.
There was a problem hiding this comment.
Sorry, I hadn't processed the entire original description before making this review comment.
pkg/commands/set_instancegroup.go
Outdated
|
|
||
| // ValidInstanceGroupKeysToSetters returns the valid keys and config setting | ||
| // logic for instance groups. | ||
| func ValidInstanceGroupKeysToSetters() InstanceGroupKeySetters { |
There was a problem hiding this comment.
This really should be using reflection on api.InstanceGroup and its subfields. Open-coding all that is limiting and error prone.
There was a problem hiding this comment.
As I mentioned in my description, I totally agree. Would it make sense to add a few more fields with the current PR description along with more tests and then do a follow-up PR to convert bot set instancegroup and set cluster to use reflection-based config setting, or would you like that to be in this PR?
There was a problem hiding this comment.
If I were to do it I would first convert set cluster to reflection, then add set instancegroup. The reflection code should be a separate commit (or commits), if not PR.
There was a problem hiding this comment.
That totally makes sense. Does the following sound like a reasonable compromise?
- Merge this PR with current implementation, but more fields and tests.
- Add reflection code and move
set clusterto use this, using original tests to help verify no regressions. - Move
set instancegroupto use reflection and use original tests to help verify no regressions.
If not, I can drop/change this PR to work on the reflection code again.
There was a problem hiding this comment.
I wouldn't block that approach, not that I could anyway. The primary downside would be the cost of reviewing code that is likely to be ripped out shortly. The risk of step 2 not happening for a while means review standards on it can't be lowered.
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
I think it's fine to implement a subset of fields through reflection. If a leaf field has an unsupported type, emit an error. |
|
/retest |
|
I had some time to start writing code to handle running Let me know how you would like me to proceed @rdrgmnzs or others. Thanks. |
|
My preference is for (4). That would be the most efficient use of my review time. |
|
Okay, assuming I don't get other input I will start working on a separate PR for |
| @@ -17,8 +17,7 @@ limitations under the License. | |||
| package commands | |||
|
|
|||
| import ( | |||
| "fmt" | |||
|
|
|||
| "github.com/pkg/errors" | |||
There was a problem hiding this comment.
FYI I think we should use the functionality in xerrors which did make it into go 1.13, specifically fmt.Errorf("foo: %w", err)
|
I'd actually like to see us merge this as-is, though I agree that we shouldn't be printing the list of supported fields. I was testing out spot instance support (I think it is broken with launch templates), and this would have been handy. A reflection-based approach is better, but it's also a harder in general which is why we haven't done it so far. There's still an open question about what syntax we should use, but that was one of the goals of hand-coding these; it looks like I think we can support a subset of jmespath which is only the Anyway, as it's not trivial to write the reflection code, I'd be in favor of merging this (minus the list of all fields), and then we can implement the reflection. @johngmyers do you agree? @gabrieljackson did you make any progress with the reflection code? And would you be able to update this to not print the list of fields? |
|
Aside from the risk of having to maintain quirks due to variance between reflection syntax and hand-coded syntax, I'm not a fan of stopgap code. There's nothing as permanent as temporary measures. |
k8s-ci-robot
added
size/L
|
Sorry for the delay in response @justinsb. I have begun working on the reflect code and have it partially implemented, but am still working through the brittleness that comes along with reflect. As you mentioned above it's "harder in general" and I have been thinking about the method with which we decide to match a given All that said, I am still working on it, but it is becoming quite large and will take a good while longer before I can prioritize it again. In the meantime, I have pushed up some commits here to catch this PR up to master and to remove the field list functionality as requested. If there is a change of heart and we decide this is worth merging as is I would be happy to work to get that done soon. I still think there is value to merging this as-is, but acknowledge that it would require reviewing some amount of temporary code. |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
|
@gabrieljackson: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@gabrieljackson now that the reflection code has landed in #8896 could you recode this to be in terms of that? |
|
@johngmyers, I can absolutely do that. Thank you for letting me know that someone else had time to get the reflection logic in. |
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
needs-rebase
|
@gabrieljackson: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@fejta-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/reopen |
|
@gabrieljackson: Failed to re-open PR: state cannot be changed. The set-instancegroups-cmd branch was force-pushed or recreated. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This change adds a new command and functionality for updating
instance group configuration via command line arguments. This
behavior mimics the
set clustercommand.Being able to update the instance group image via automation
would be very helpful to us: #7976
Regarding the implementation, I understand that the
set clustercommand was done with hardcoded values with the intention of
updating it to "something better" in the long run. I believe I took
a step in that direction with the way I constructed this command.
By using a map of setting keys and config update logic (setters)
it's a little easier to do things like use the same setter function on
multiple instance groups in the future and to see what fields are
available to update via helper methods. If the reviewers like this
approach, I would be happy to do the same thing for the
set clustercommand. It should be noted that I originally tried todo reflect lookups on the actual config struct, but I found my
initial attempts to be brittle and the code to be confusing so
I opted for this instead. If this isn't a good initial implementation
then I am more than willing to swap it out.
Lastly, if the implementation is good enough, then I will go back
and add more instance group fields that can be set. I didn't want
to do that before I knew what initial reviewers thought.
Thanks!