Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Spotinst: Avoid spurious changes #6028

Merged
merged 3 commits into from
Mar 15, 2019
Merged

Spotinst: Avoid spurious changes #6028

merged 3 commits into from
Mar 15, 2019

Conversation

liranp
Copy link
Contributor

@liranp liranp commented Oct 31, 2018

This PR sets additional fields to avoid spurious changes. Otherwise, we were seeing those fields changing every time.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 31, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @liranp. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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 k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 31, 2018
justinsb
justinsb reviewed Nov 19, 2018
View reviewed changes
upup/pkg/fi/cloudup/spotinsttasks/elastigroup.go Outdated
@@ -134,7 +135,12 @@ func (e *Elastigroup) Find(c *fi.Context) (*Elastigroup, error) {
{
for _, zone := range compute.AvailabilityZones {
if zone.SubnetID != nil {
actual.Subnets = append(actual.Subnets, &awstasks.Subnet{ID: zone.SubnetID})
t := &awstasks.Subnet{ID: zone.SubnetID}
s, err := t.Find(c)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change shouldn't be necessary - Subnet implements CompareWithID and returns ID as the comparison value https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/awstasks/subnet.go#L55 . When we're comparing fields for changes, we should therefore consider two Subnets with the same ID the same, even if the other fields are different.

But likely I just don't understand why you need to populate the other fields?

upup/pkg/fi/cloudup/spotinsttasks/elastigroup.go Outdated
image, err := resolveImage(cloud, fi.StringValue(lc.ImageID))
if err != nil {
return nil, err
if e.ImageID != nil && lc.ImageID != nil &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we won't set actual.ImageID in e.g. the case when e.ImageId == lc.ImageID. I took a look at how "raw AWS" does it - it sets actual.ImageID = lc.ImageID. Then it tries to avoid spurious changes by doing this logic. But you do need to continue to set ImageID in the common case.

upup/pkg/fi/cloudup/spotinsttasks/elastigroup.go Outdated
if s != nil {
for _, sgID := range lc.SecurityGroupIDs {
if fi.StringValue(s.ID) == sgID {
actual.SecurityGroups = append(actual.SecurityGroups, s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we're driving this from the expected values, and ignoring other security groups we find on the LaunchConfiguration. Not sure what we're doing here, and I'm not sure it's right. If it is right it does need a comment because it's unusual..

upup/pkg/fi/cloudup/spotinsttasks/elastigroup.go Outdated
if err != nil {
return nil, err
}
actual.IAMInstanceProfile = s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with the Subnets, Name is the comparison field, and so I'm not sure why this is necessary

upup/pkg/fi/cloudup/spotinsttasks/elastigroup.go Outdated
actual.LoadBalancer = &awstasks.LoadBalancer{
Name: lbs[0].Name,
LoadBalancerName: lbs[0].Name,
t := &awstasks.LoadBalancer{Name: e.LoadBalancer.Name}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with Subnets, Name is the CompareWithID field, so not sure why this is needed.

upup/pkg/fi/cloudup/spotinsttasks/elastigroup.go Outdated
if lc.KeyPair != nil {
actual.SSHKey = &awstasks.SSHKey{Name: lc.KeyPair}
t := &awstasks.SSHKey{Name: lc.KeyPair}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name should be the key field here as well, so this shouldn't be needed

@justinsb
Copy link
Member

/ok-to-test

Mostly good, but I'm a little unclear why we need to do a Find on the Subnets etc. Is the comparison logic showing these as changes (when they aren't changing)?

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 19, 2018
@liranp
Copy link
Contributor Author

liranp commented Nov 29, 2018

@justinsb Sorry for the delay in replying, I've fixed the code according to you comments.

@liranp
Copy link
Contributor Author

liranp commented Dec 18, 2018

@justinsb Can you please review the changes?

@liranp
Copy link
Contributor Author

liranp commented Jan 7, 2019

@justinsb Sorry for bothering you again. Any news?

@liranp
Copy link
Contributor Author

liranp commented Jan 30, 2019 •
edited
Loading

@justinsb / @rdrgmnzs Can you please review or assign it to another reviewer? Thanks!

@liranp
Copy link
Contributor Author

liranp commented Feb 5, 2019

/assign @chrislovecnm

Any chance that you can review it? We already have additional resources and changes that we would like to push but we are waiting for this PR to be merged in to avoid conflicts. Thanks!

@liranp
Copy link
Contributor Author

liranp commented Feb 13, 2019

@robinpercy Can you please assist?

@liranp
Copy link
Contributor Author

liranp commented Feb 20, 2019

@justinsb / @rdrgmnzs / @robinpercy / @chrislovecnm Bumping this back up...

@justinsb
Copy link
Member

Ouch - sorry for the delay on this one! One thing we could do is to make you an OWNER on spotinsttasks, so that spotinst specific changes will get merged faster. But this one also got hit by the double-trouble of the test jobs and then the docker CVE, so hopefully we won't have this much lag in future..

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, liranp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2019
@k8s-ci-robot k8s-ci-robot merged commit 7f6157d into kubernetes:master Mar 15, 2019
@liranp liranp deleted the fix-update-cluster branch May 5, 2019 14:36
@liranp liranp mentioned this pull request Feb 17, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

@rdrgmnzs rdrgmnzs Awaiting requested review from rdrgmnzs

@robinpercy robinpercy Awaiting requested review from robinpercy

Assignees

@justinsb justinsb

@chrislovecnm chrislovecnm

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@liranp @k8s-ci-robot @justinsb @chrislovecnm