Bumped dependencies #1420

zen-dog · 2020-03-11T23:17:07Z

Summary:

controller-runtime to 0.5.1
kubectl to 0.17.3

Gotchas:
For some harness tests, I had to edit our test CRDs like:

-  name: mycrds.mycrd.k8s.io
+  name: mycrds.mycrd.example.com

With new kubectl version, the original name had multiple problems:

names ending with k8s.io are restricted and now require an api-approved.kubernetes.io annotation pointing to a PR which introduced them
adding this annotation proved challenging since we then see a new transition condition with the reason: ApprovedAnnotation thus bringing the number of status.condition s to three. These conditions would sometimes arrive out of order, however, test harness does not support the same order of elements when comparing two arrays, thus making the tests flaky. See this thread for more details:

Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com

zen-dog · 2020-03-11T23:18:30Z

pkg/kudoctl/util/kudo/kudo_test.go

-		{bool: true, createns: "default", obj: &obj}, // 4
-		{getns: "kudo", obj: &obj},                   // 4
+		{exists: true, obj: &obj},               // 3
+		{exists: true, createns: "default", obj: &obj, getns: "default"}, // 4


This test was failing without explicitly using namespace default.

zen-dog · 2020-03-11T23:18:57Z

pkg/test/step_test.go

@@ -32,7 +33,7 @@ func TestStepClean(t *testing.T) {
 	pod2WithNamespace := testutils.NewPod("hello2", testNamespace)
 	pod2WithDiffNamespace := testutils.NewPod("hello2", "different-namespace")

-	cl := fake.NewFakeClient(pod, pod2WithNamespace, pod2WithDiffNamespace)
+	cl := fake.NewFakeClientWithScheme(scheme.Scheme, pod, pod2WithNamespace, pod2WithDiffNamespace)


NewFakeClient method is deprecated

zen-dog · 2020-03-11T23:19:17Z

pkg/test/step_test.go

@@ -53,16 +54,16 @@ func TestStepClean(t *testing.T) {
 // Each test provides a path to a set of test steps and their rendered result.
 func TestStepCreate(t *testing.T) {

-	pod := testutils.NewPod("hello", "")
+	pod := testutils.NewPod("hello", "default")


another case where a test started to fail without an explicit default namespace.

🤔 I feel like this behavior as tested is wrong. It should only be setting the namespace if it isn't set. Do you know what changed?

So, for anybody who is interested: after some researching and debugging, I believe it was this change in go-client testing fixture that made this test fail. The commit description says it all.
As for the default client, this is also explicitly forbidden. Another mystery solved.

zen-dog · 2020-03-11T23:19:58Z

pkg/test/test_data/crd-in-step/00-crd.yaml

@@ -3,6 +3,8 @@ apiVersion: apiextensions.k8s.io/v1beta1
 kind: CustomResourceDefinition
 metadata:
  name: mycrds.mycrd.k8s.io
+  annotations:
+    api-approved.kubernetes.io: "https://github.com/kubernetes/enhancements/pull/1111"


See the PR in the annotation for more details on why this is needed

jbarrick-mesosphere · 2020-03-11T23:20:33Z

pkg/test/test_data/crd-in-step/00-assert.yaml

@@ -11,8 +11,3 @@ status:
    singular: mycrd
  storedVersions:
  - v1beta1
-  conditions:


Did something in the CRD API change?

see my comment below

jbarrick-mesosphere · 2020-03-11T23:24:06Z

pkg/test/step_test.go

@@ -53,16 +54,16 @@ func TestStepClean(t *testing.T) {
 // Each test provides a path to a set of test steps and their rendered result.
 func TestStepCreate(t *testing.T) {

-	pod := testutils.NewPod("hello", "")
+	pod := testutils.NewPod("hello", "default")


🤔 I feel like this behavior as tested is wrong. It should only be setting the namespace if it isn't set. Do you know what changed?

zen-dog · 2020-03-11T23:26:55Z

pkg/test/test_data/crd-in-step/00-assert.yaml

@@ -11,8 +11,3 @@ status:
    singular: mycrd
  storedVersions:
  - v1beta1
-  conditions:


We have one more condition now:

- type: KubernetesAPIApprovalPolicyConformant reason: ApprovedAnnotation status: "True"

and with that, the test became flaky. I'm not sure if it takes a while for the transition to happen or smth. else

Ok, so I've got to the bottom of it (which in the hindsight wasn't very deep): with three conditions, they sometimes simply come out of order and test-harness expects the same order of elements when comparing slices in the IsSubset method. A quick test confirms:

assert.Nil(t, IsSubset( []string{"foo", "bar"}, []string{"bar", "foo"}, )) fails with: Expected nil, but got: &utils.SubsetError{path:[]string(nil), message:"value mismatch, expected: foo != actual: bar"}

However, it turns out we can not omit the conditions here:

the first step creates the CRD and assumes that they're created

the second step creates a CR of that CRD

but the second create-step (not the assert one) occasionally fails with the server could not find the requested resource because the CRD hasn't gone through all transitions yet.

I'd suggest fixing IsSubset comparing slices but I'm not sure about all the implications. /cc @justinbarrick

Yeah, right now it is order dependent, I need to evaluate existing tests to see if order is often important. IIRC @alenkacz and I discussed this a while back but I don't remember where we landed on it.

Ok, so I avoided the problem for this bump by changing the CRD name/group to prowjobs.prow.example.com (instead of *.k8s.io) which doesn't require the annotation and so we only have the same two conditions as before (these two do not come out of order).

However, the bigger question stays: I'm not sure if k8s guarantee array elements order in all places. We might want to relax the current subset comparison to be order independent or give the user the choice to use both (some sort of annotation maybe?)

zen-dog · 2020-03-11T23:27:08Z

pkg/test/test_data/create-or-update/00-assert.yaml

@@ -5,13 +5,5 @@ metadata:
 status:
  acceptedNames:
    kind: ProwJob
-  # InitialNamesAccepted will change from False -> True once the CRD is ready.


alenkacz · 2020-03-12T07:53:52Z

pkg/test/utils/kubernetes.go

+	"--insecure-port={{ if .URL }}{{ .URL.Port }}{{ end }}",
+	"--insecure-bind-address={{ if .URL }}{{ .URL.Hostname }}{{ end }}",
+	"--secure-port={{ if .SecurePort }}{{ .SecurePort }}{{ end }}",
+	"--admission-control=AlwaysAdmit",


I wrote you a lenghty comment on slack so just re-posting part of it here because this won't work:

if you're interested in the details - before the admission plugins were disabled altogether with AlwaysAdmiy, that was the default behavior of envtest, so no admission plugin was ever hit in tests (I think that is wrong anyway and we plan to change it in 0.6.0). But I needed to touch that anyway because I needed to enable webhook admission plugins in order for webhooks to work. So I had to drop AlwaysAdmit and that might be the reason. I instead explicitly disabled all admission plugins that are enabled by default but maybe that's not exactly 1:1 and might be causing you some troubles (edited)

I mean it's fine for the bump, but not for the webhooks test. It's also not fine in general because you're ignoring all kinds of real apiserver errors that normally happen

and kubectl to 0.17.3 Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

…ator` Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

…rguments" This reverts commit a7b1150. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

…ersion Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

….example.com` this will avoid the problem of having to compare slices independent of elements order since we wont have the annotation condition present. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

go.mod

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

porridge

Please mention why we needed this bump in PR description.

Also, I'd suggest changing:

Bumped controller-runtime to 0.5.1

and kubectl to 0.17.3

To something like:

Bumped dependencies.

- controller-runtime to 0.5.1
- kubectl to 0.17.3

Finally, if you feel like it, it would be great if you could summarize the gotchas in the PR description for posterity. Right now it can take a while to hunt them down and realize there was a slight change of approach on the .k8s.io issue.

pkg/kudoctl/util/kudo/kudo_test.go

pkg/test/test_data/crd-in-step/00-assert.yaml

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

ANeumann82

lgtm

zen-dog requested review from alenkacz, gerred, jbarrick-mesosphere, kensipe and nfnt as code owners March 11, 2020 23:17

zen-dog commented Mar 11, 2020

View reviewed changes

jbarrick-mesosphere reviewed Mar 11, 2020

View reviewed changes

zen-dog commented Mar 11, 2020

View reviewed changes

alenkacz reviewed Mar 12, 2020

View reviewed changes

zen-dog added 6 commits March 13, 2020 15:16

Bumped controller-runtime to 0.5.1

7f3273a

and kubectl to 0.17.3 Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Making linter happy

0fa5990

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Removed admission-control=AlwaysAdmit option from default arguments

485ad50

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Making linter happy again

ba051a3

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Proper code-generator location

fa8fda2

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Updated generated files with the new version of the `controller-gener…

040243e

…ator` Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog force-pushed the ad/bump-controller-runtime-0.5.1 branch from 2c1ef5b to 579263d Compare March 13, 2020 14:17

zen-dog added 3 commits March 13, 2020 15:31

Revert "Removed admission-control=AlwaysAdmit option from default a…

cd8b917

…rguments" This reverts commit a7b1150. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Updated golden files

0b88fc2

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

APIServerDefaultArgs are now as they were defined in the previous v…

06fafe0

…ersion Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog force-pushed the ad/bump-controller-runtime-0.5.1 branch from 579263d to 06fafe0 Compare March 13, 2020 14:31

porridge requested changes Mar 16, 2020

View reviewed changes

go.mod Outdated Show resolved Hide resolved

Using proper patched code-generator version

17fe27a

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

porridge approved these changes Mar 17, 2020

View reviewed changes

pkg/kudoctl/util/kudo/kudo_test.go Show resolved Hide resolved

pkg/test/test_data/crd-in-step/00-assert.yaml Show resolved Hide resolved

zen-dog changed the title ~~Bumped controller-runtime to 0.5.1~~ Bumped dependencies Mar 17, 2020

Removed a unicode emoji remnant from manager logs

87e198c

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

ANeumann82 approved these changes Mar 17, 2020

View reviewed changes

zen-dog merged commit 452c5b6 into master Mar 18, 2020

zen-dog deleted the ad/bump-controller-runtime-0.5.1 branch March 18, 2020 10:47

This was referenced Mar 20, 2020

Bump Dependencies kudobuilder/kuttl#25

Merged

Order Dependency Solution kudobuilder/kuttl#27

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bumped dependencies #1420

Bumped dependencies #1420

zen-dog commented Mar 11, 2020 •

edited

Loading

zen-dog Mar 11, 2020

zen-dog Mar 11, 2020

zen-dog Mar 11, 2020

jbarrick-mesosphere Mar 11, 2020

zen-dog Mar 13, 2020 •

edited

Loading

zen-dog Mar 11, 2020

jbarrick-mesosphere Mar 11, 2020

zen-dog Mar 12, 2020

jbarrick-mesosphere Mar 11, 2020

zen-dog Mar 11, 2020

zen-dog Mar 13, 2020 •

edited

Loading

jbarrick-mesosphere Mar 13, 2020

zen-dog Mar 16, 2020

zen-dog Mar 11, 2020

alenkacz Mar 12, 2020 •

edited by porridge

Loading

alenkacz Mar 12, 2020

porridge left a comment

ANeumann82 left a comment

Bumped dependencies #1420

Bumped dependencies #1420

Conversation

zen-dog commented Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zen-dog Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zen-dog Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenkacz Mar 12, 2020 • edited by porridge Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porridge left a comment

Choose a reason for hiding this comment

ANeumann82 left a comment

Choose a reason for hiding this comment

zen-dog commented Mar 11, 2020 •

edited

Loading

zen-dog Mar 13, 2020 •

edited

Loading

zen-dog Mar 13, 2020 •

edited

Loading

alenkacz Mar 12, 2020 •

edited by porridge

Loading