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

various operator fixes #830

Merged
merged 2 commits into from
Dec 17, 2020
Merged

various operator fixes #830

merged 2 commits into from
Dec 17, 2020

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Dec 16, 2020

This covers validation, defaulting and conversion.

@pohly pohly requested a review from avalluri December 16, 2020 13:22
@pohly pohly mentioned this pull request Dec 16, 2020
Merged
3 tasks
@pohly
Copy link
Contributor Author

pohly commented Dec 16, 2020

I removed validateDriver calls in some tests because that slowed down testing and didn't fit the purpose of the test.

Now it seems that sometimes something gets stuck: https://cloudnative-k8sci.southcentralus.cloudapp.azure.com/blue/organizations/jenkins/pmem-csi/detail/PR-830/1/tests

Deleting the CR hangs and eventually times out. I saw that once also locally, but couldn't reproduce it.

Any idea why it might get stuck?

@pohly
Copy link
Contributor Author

pohly commented Dec 16, 2020

Nope, not related to changes in this PR. It also fails the same way in https://cloudnative-k8sci.southcentralus.cloudapp.azure.com/blue/organizations/jenkins/pmem-csi/detail/PR-825/6/tests

@avalluri
Copy link
Contributor

Nope, not related to changes in this PR. It also fails the same way in https://cloudnative-k8sci.southcentralus.cloudapp.azure.com/blue/organizations/jenkins/pmem-csi/detail/PR-825/6/tests

The reals issue is with running the earlier test:

 Dec 16 18:06:18.683: INFO: create deployment error: object is being deleted: deployments.pmem-csi.intel.com "alpha-default-values" already exists, will retry...
[2020-12-16T18:06:20.086Z] Dec 16 18:06:19.639: FAIL: Timed out after 180.000s.
[2020-12-16T18:06:20.087Z] create deployment "alpha-default-values"
[2020-12-16T18:06:20.087Z] Expected
[2020-12-16T18:06:20.087Z]     <*errors.StatusError | 0xc0019faa00>: {
[2020-12-16T18:06:20.087Z]         ErrStatus: {
[2020-12-16T18:06:20.087Z]             TypeMeta: {Kind: "Status", APIVersion: "v1"},
[2020-12-16T18:06:20.087Z]             ListMeta: {
[2020-12-16T18:06:20.087Z]                 SelfLink: "",
[2020-12-16T18:06:20.087Z]                 ResourceVersion: "",
[2020-12-16T18:06:20.087Z]                 Continue: "",
[2020-12-16T18:06:20.087Z]                 RemainingItemCount: nil,
[2020-12-16T18:06:20.087Z]             },
[2020-12-16T18:06:20.087Z]             Status: "Failure",
[2020-12-16T18:06:20.087Z]             Message: "object is being deleted: deployments.pmem-csi.intel.com \"alpha-default-values\" already exists",
[2020-12-16T18:06:20.087Z]             Reason: "AlreadyExists",
[2020-12-16T18:06:20.087Z]             Details: {
[2020-12-16T18:06:20.087Z]                 Name: "alpha-default-values",
[2020-12-16T18:06:20.087Z]                 Group: "pmem-csi.intel.com",
[2020-12-16T18:06:20.087Z]                 Kind: "deployments",
[2020-12-16T18:06:20.087Z]                 UID: "",
[2020-12-16T18:06:20.087Z]                 Causes: nil,
[2020-12-16T18:06:20.087Z]                 RetryAfterSeconds: 0,
[2020-12-16T18:06:20.087Z]             },
[2020-12-16T18:06:20.087Z]             Code: 409,
[2020-12-16T18:06:20.087Z]         },
[2020-12-16T18:06:20.087Z]     }
[2020-12-16T18:06:20.087Z] to be nil

@pohly
Copy link
Contributor Author

pohly commented Dec 17, 2020

The reals issue is with running the earlier test:
[...]
Same problem, somehow CR deletion got stuck.

@pohly
operator: various CRD fixes (defaults, validation)
053e442 
Various markup fields were treated as part of the field description.
We don't want the API server to set defaults for us because then they
become part of the API version which defines them. Instead, we want
the flexibility to change the default over time when that makes sense.
@pohly
operator: fix conversion web hook
2a9c10f 
Not all fields were getting copied. The test was incomplete and
therefore didn't catch that mistake.
avalluri
avalluri reviewed Dec 17, 2020
View reviewed changes
pkg/apis/pmemcsi/v1alpha1/deployment_conversion.go
out.Spec.Image = in.Spec.Image
out.Spec.CACert = in.Spec.CACert
out.Spec.PullPolicy = in.Spec.PullPolicy
Copy link
Contributor

@avalluri avalluri Dec 17, 2020 •
edited

Choose a reason for hiding this comment

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

@pohly I googled about this missing conversions and came across about conversion-gen code generation tool, that generates the type conversion code as mentioned here. What do you think, shall we use that as a separate change or part of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we use it, then in a separate PR.

I want to get into a stable state as quickly as possible again and using that tool may need further investigations and discussions.

@pohly pohly mentioned this pull request Dec 17, 2020
Merged
@pohly pohly merged commit 2a9c10f into intel:devel Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avalluri avalluri avalluri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pohly @avalluri