Add a test to check for machineset leftovers ports #45

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

Merged

openshift-merge-robot merged 3 commits into openshift:main from itzikb-redhat:machineset

Sep 14, 2022

Contributor

itzikb-redhat commented Aug 4, 2022

Create a machineset with a bogus serverGroupID
Delete the machineset
Check that there is no leftover port associated with the deleted machineset

openshift-ci bot requested review from EmilienM and MaysaMacedo

August 4, 2022 13:21

Contributor

rlobillo commented Aug 4, 2022 •

edited

Loading

Hello @itzikb-redhat I think you can move the test to servers.go file. For the files I created until now, I tried to give "openstack" perspective (servers, servergroups, volumes...) and the operation you are testing is related to the creation of a new server. What do you think?

itzikb-redhat force-pushed the machineset branch 2 times, most recently from 5192ae4 to fc4e3c5 Compare

August 4, 2022 14:09

Contributor Author

itzikb-redhat commented Aug 4, 2022

Hello @itzikb-redhat I think you can move the test to servers.go file. For the files I created until now, I tried to give "openstack" perspective (servers, servergroups, volumes...) and the operation you are testing is related to the creation of a new server. What do you think?

I think it's more related to machinesets but we can discuss

itzikb-redhat force-pushed the machineset branch 12 times, most recently from 1f7ca21 to 0e4c385 Compare

August 8, 2022 09:37

rlobillo reviewed

View reviewed changes

Contributor

rlobillo left a comment

Great job @itzikb-redhat

I think it would be very useful in this complex testcase to make use of g.By. With this utility it is easier for maintainers to understand the different stages in the testcase. Would you mind to include them in the test? This will help for review too.

Also, in the Expect commands, it is possible to include a failure message:

o.Expect(err).NotTo(o.HaveOccurred(), "Error unmarshaling new MachineSet Specs")

I think that is very useful to get a clear view of the failure by only checking the logs.

[0] https://onsi.github.io/ginkgo/#documenting-complex-specs-by

test/extended/openstack/machinesets.go Outdated

+              			g.By("fetching worker machineSets")
+              			var networkClient *gophercloud.ServiceClient
+              			var rawBytes []byte
+              			var t machineproviderv1.OpenstackProviderSpec

Contributor

rlobillo Aug 23, 2022 •

edited

Loading

Could you please use a more descriptive name for the variable on line 47?

Contributor Author

itzikb-redhat Aug 23, 2022

Done

test/extended/openstack/machinesets.go

+              			rclient, err = runtimeclient.New(xconfig, runtimeclient.Options{})
+              			o.Expect(err).NotTo(o.HaveOccurred())
+              			err = machinev1.AddToScheme(scheme.Scheme)

Contributor

rlobillo Aug 23, 2022 •

edited

Loading

Please include a comment stating why you need to do this (line 74).

MaysaMacedo reviewed

View reviewed changes

Contributor

MaysaMacedo left a comment

Is this test an outcome of a bugzilla?

test/extended/openstack/machinesets.go Outdated

+              			o.Expect(err).NotTo(o.HaveOccurred())
+              			clientSet, err = e2e.LoadClientset()
+              			o.Expect(err).NotTo(o.HaveOccurred())
+              			g.By("preparing openstack client")

Contributor

MaysaMacedo Aug 23, 2022

this line and the next one are unnecessary

Contributor Author

itzikb-redhat Aug 23, 2022

Done

test/extended/openstack/machinesets.go Outdated

+              			if len(machineSets) == 0 {
+              				e2eskipper.Skipf("Expects at least one worker machineset. Found none.")
+              			}
+              			xconfig, err := config.GetConfig()

Contributor

MaysaMacedo Aug 23, 2022

Is it needed to get the config again if it's already available at line 40 or it's a different one?

Contributor Author

itzikb-redhat Aug 23, 2022

I'm changing to e2e.LoadConfig , anyway cfg is not accessible here

test/extended/openstack/machinesets.go Outdated

+              			newMachinesetParams := framework.BuildMachineSetParams(rclient, 1)
+              			rawBytes, err = json.Marshal(newMachinesetParams.ProviderSpec.Value)
+              			err = json.Unmarshal(rawBytes, &t)

Contributor

MaysaMacedo Aug 23, 2022

what t represents? a machineset?

Contributor Author

itzikb-redhat Aug 23, 2022

I modified the name to newProviderSpec, It's the providerspec of the machineset

test/extended/openstack/machinesets.go Outdated

+              			s, err := json.Marshal(t)
+              			newMachinesetParams.ProviderSpec.Value.Raw = s
+              			o.Expect(err).NotTo(o.HaveOccurred())

Contributor

MaysaMacedo Aug 23, 2022

it might be better to move this line to after 88 so we can easily identify which err is being checked

Contributor Author

itzikb-redhat Aug 23, 2022

Done

test/extended/openstack/machinesets.go

+              			newMachinesetParams.ProviderSpec.Value.Raw = s
+              			o.Expect(err).NotTo(o.HaveOccurred())
+              			ms, err := framework.CreateMachineSet(rclient, newMachinesetParams)

Contributor

MaysaMacedo Aug 23, 2022

Suggested change

      
            			ms, err := framework.CreateMachineSet(rclient, newMachinesetParams)
          
                                    g.By("Create machineSet with new name and server group")
          
            			ms, err := framework.CreateMachineSet(rclient, newMachinesetParams)

Contributor Author

itzikb-redhat Aug 23, 2022 •

edited

Loading

Added a similar one

test/extended/openstack/machinesets.go

+              			portID := ""
+              			for _, port := range allPorts {
+              				if strings.Contains(port.Name, newMachinesetParams.Name) {

Contributor

MaysaMacedo Aug 23, 2022

is the name of the port exactly a fragment of the machineset name or it should be equal?
If it's equal, we could filter the ports query by that name as well

Contributor Author

itzikb-redhat Aug 23, 2022

The ports contains the string but it's not exact

test/extended/openstack/machinesets.go Outdated

+              			o.Expect(err).NotTo(o.HaveOccurred())
+              			//Delete the machineset
+              			e2e.Logf("Waiting for 10 seconds before deleting machineset %v", ms.Name)

Contributor

MaysaMacedo Aug 23, 2022

What happens if 10 seconds was not enough to create the machine? Perhaps we should poll instead

Contributor Author

itzikb-redhat commented Aug 23, 2022

@MaysaMacedo BZ2073398

Contributor

MaysaMacedo commented Aug 23, 2022

@MaysaMacedo BZ2073398

@itzikb-redhat It would be good to add a comment to the code, linking that bz. Thanks

Contributor Author

itzikb-redhat commented Aug 23, 2022

@MaysaMacedo BZ2073398

@itzikb-redhat It would be good to add a comment to the code, linking that bz. Thanks

Done

itzikb-redhat force-pushed the machineset branch from 0e4c385 to 8cc617b Compare

August 23, 2022 13:00

openshift-merge-robot added the needs-rebase label

itzikb-redhat force-pushed the machineset branch from 8cc617b to 9e7c08b Compare

August 23, 2022 16:27

Member

pierreprinetti commented Aug 24, 2022

/test verify

pierreprinetti force-pushed the machineset branch from 9e7c08b to 5fe1085 Compare

August 24, 2022 11:35

openshift-merge-robot removed the needs-rebase label

Member

pierreprinetti commented Aug 24, 2022

Rebased on main

Contributor Author

itzikb-redhat commented Sep 4, 2022

/retest

Contributor

dulek commented Sep 5, 2022

/lgtm

openshift-ci bot added the lgtm label

Contributor

MaysaMacedo commented Sep 6, 2022

/retest
/approve

openshift-ci bot added the approved label

openshift-ci-robot commented Sep 6, 2022

/retest-required

Remaining retests: 0 against base HEAD e334265 and 2 for PR HEAD 64066ce in total

Contributor

MaysaMacedo commented Sep 6, 2022

/hold
CCM was not deployed, likely depends on openshift/cluster-cloud-controller-manager-operator#202

openshift-ci bot added the do-not-merge/hold label

Contributor Author

itzikb-redhat commented Sep 7, 2022

/retest

1 similar comment

Contributor Author

itzikb-redhat commented Sep 7, 2022

/retest

Contributor

MaysaMacedo commented Sep 7, 2022

/hold cancel
dependent PR was merged

openshift-ci bot removed the do-not-merge/hold label

Contributor

MaysaMacedo commented Sep 7, 2022

/approve remove
seems other test is being impacted

Contributor

MaysaMacedo commented Sep 7, 2022

/approve cancel

openshift-ci bot removed the approved label


          Check addresses only in found machines

eb55bcc

In the following test

"[sig-installer][Suite:openshift/openstack] The OpenStack platform on instance creation should include the addresses on the machine specs [Suite:openshift/conformance/parallel]"

it may happen that a machine that was created by another test is gone, we add a check so that only found machines are tested

itzikb-redhat force-pushed the machineset branch from 64066ce to eb55bcc Compare

September 7, 2022 16:30

openshift-ci bot removed the lgtm label

Contributor Author

itzikb-redhat commented Sep 7, 2022

So "[sig-installer][Suite:openshift/openstack] The OpenStack platform on instance creation should include the addresses on the machine specs [Suite:openshift/conformance/parallel]"
failed

The updated PS should solve it for now but I wonder if the test shouldn't be modified to reflect the bug , i.e. to add a FIP to a machine and then check the addresses of the machine instead of going over all the machines

Contributor

openshift-ci bot commented Sep 7, 2022

@itzikb-redhat: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

Contributor

MaysaMacedo commented Sep 14, 2022

/lgtm
/approve

openshift-ci bot assigned MaysaMacedo

openshift-ci bot added the lgtm label

Contributor

openshift-ci bot commented Sep 14, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MaysaMacedo

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [MaysaMacedo]

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

openshift-ci bot added the approved label

openshift-merge-robot merged commit f89c71b into openshift:main

pierreprinetti deleted the machineset branch

September 14, 2022 08:28

Contributor Author

itzikb-redhat commented Sep 14, 2022

/cherry-pick release-4.11

openshift-cherrypick-robot commented Sep 14, 2022

@itzikb-redhat: #45 failed to apply on top of branch "release-4.11":

Applying: Add/Update packages needed for testing machineset leftovers
.git/rebase-apply/patch:3316: space before tab in indent.
    	return client.FetchCount()
.git/rebase-apply/patch:3352: space before tab in indent.
    	model, err := client.Find(1138)
.git/rebase-apply/patch:3353: space before tab in indent.
    	g.Expect(err).NotTo(HaveOccurred())
.git/rebase-apply/patch:3354: space before tab in indent.
    	g.Expect(model.Reticulate()).To(Succeed())
.git/rebase-apply/patch:3355: space before tab in indent.
    	g.Expect(model.IsReticulated()).To(BeTrue())
error: patch failed: vendor/github.com/MakeNowJust/heredoc/README.md:1
error: vendor/github.com/MakeNowJust/heredoc/README.md: patch does not apply
error: Did you hand edit your patch?
It does not apply to blobs recorded in its index.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Using index info to reconstruct a base tree...
M	go.mod
M	go.sum
M	vendor/modules.txt
Patch failed at 0001 Add/Update packages needed for testing machineset leftovers
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherry-pick release-4.11

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.

pierreprinetti mentioned this pull request

[release-4.11] Add a test to check for machineset leftovers ports #52

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels