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.

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

✨(kustomize/v1, go/v3) : Add new LeaderElectionReleaseOnCancel as true option commented to let users know when they can use it #2596

Merged

Conversation

NikhilSharmaWe
Copy link
Member

Fixes #2593

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 3, 2022
@NikhilSharmaWe
Copy link
Member Author

NikhilSharmaWe commented Apr 3, 2022

@joelanford @camilamacedo86 @AlmogBaku
Error logs says vet: ./main.go:64:3: unknown field LeaderElectionReleaseOnCancel in struct literal. Unable to understand the cause of it. The function NewManager uses type Options as one of the parameters, since Options is actually manager.Options and since it has LeaderElectionReleaseOnCancel field in it why this error is being displayed.

@AlmogBaku
Copy link
Member

This feature wasn't supported in the controller-runtime:v0.6.4 which is the version that the go/v2 plugin uses - therefore, you should only patch go/v3

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 4, 2022
@@ -238,13 +238,16 @@ func main() {
ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts)))

{{ if not .ComponentConfig }}
// Setting LeaderElectionReleaseOnCancel to true prevents main.go
// scripts to close when Manager is done and improves the performance of re-election.
Copy link
Member Author

Choose a reason for hiding this comment

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

@AlmogBaku All tests are passing now. Could you please review this explanation comment added before moving forward.

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

I think we need to discuss if this option should or NOT be added by default?
Also, what is the motivation for that?
What issue are you facing that you want to add to the default scaffold?

See : #2593 (comment)

Let's discuss it in the issue.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2022
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 28, 2022
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 28, 2022
@NikhilSharmaWe
Copy link
Member Author

NikhilSharmaWe commented May 1, 2022

@camilamacedo86 and @AlmogBaku Do we also need to add the comment here https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v3-config/main.go#L66?

How can we check that it is possible via the ComponentConfig as well (since @camilamacedo86 mentioned that this is needed for this case)?

@camilamacedo86
Copy link
Member

Hi @NikhilSharmaWe,

I think we also need to update the template used by the ComponentConfig option so that we will check its results in the sample. Why? Ensure that all have the same options, and follow the same standard making it easier for us to keep the project maintained.

@NikhilSharmaWe
Copy link
Member Author

@camilamacedo86 Are talking about adding the comment for this case as well?

options := ctrl.Options{Scheme: scheme}

@camilamacedo86
Copy link
Member

Hi @NikhilSharmaWe,

The change shows to be here:

const controllerManagerConfigTemplate = `apiVersion: controller-runtime.sigs.k8s.io/v1alpha1
kind: ControllerManagerConfig
health:
healthProbeBindAddress: :8081
metrics:
bindAddress: 127.0.0.1:8080
webhook:
port: 9443
leaderElection:
leaderElect: true
resourceName: {{ hashFNV .Repo }}.{{ .Domain }}

We need to pass the option under the leader election options
That should be also commented ( since it is an yaml file to comment you will use the prefix # )
Also, it should have the same comment explanation.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 5, 2022
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 5, 2022
@k8s-ci-robot k8s-ci-robot merged commit fc59b56 into kubernetes-sigs:master May 18, 2022
camilamacedo86 pushed a commit to camilamacedo86/operator-sdk that referenced this pull request May 29, 2022
Signed-off-by: Camila Macedo <cmacedo@redhat.com>

- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1): applying restrictive SCC for all containers scaffolded by the tool (`k8s versions < 1.19 will no longer work with`). ([More info](kubernetes-sigs/kubebuilder#2700))
- For Golang-based language (go/v2) fix the issue introduced by removing the GO111MODULE=on from Dockerfile. ([More info](kubernetes-sigs/kubebuilder#2678))
- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1 language (go/v3), add a new comment with the option `leaderElectionReleaseOnCancel` ((More info)[kubernetes-sigs/kubebuilder#2596])

Only to get the changes from what we currently use in SDK from kubebuilder.

- Ensure that scaffolds begin to be performed with a restrive Security Context
- Be able to do specific follow up PR so we can discuss properly how we will consume the new additions on Kubebuilder
- Make eaiser the process to keep SDK updated, otherwise, it is hard to properly review if we wait to do a big PR with all.
- Ensure that SDK features can work with the latest changes performed in Kubebuilder

**Extra info**
You can check the latest changes on Kubebuilder by looking: kubernetes-sigs/kubebuilder@v3.4.1...master

Note that SDK users Kubebuilder as a LIB. So that all that is currently important is updated by default when we bump. However, all that was added to Kubebuilder but still not imported/used in SDK will not be added here. Therefore we can do follow up PRs to get the additions. Why? we have significant changes and additions for example adding the support for the phase 2 plugin, but if we try to do it all in the same PR it will be very hard to get properly reviewed.
camilamacedo86 pushed a commit to camilamacedo86/operator-sdk that referenced this pull request May 29, 2022
Signed-off-by: Camila Macedo <cmacedo@redhat.com>

- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1): applying restrictive SCC for all containers scaffolded by the tool (`k8s versions < 1.19 will no longer work with`). ([More info](kubernetes-sigs/kubebuilder#2700))
- For Golang-based language (go/v2) fix the issue introduced by removing the GO111MODULE=on from Dockerfile. ([More info](kubernetes-sigs/kubebuilder#2678))
- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1 language (go/v3), add a new comment with the option `leaderElectionReleaseOnCancel` ((More info)[kubernetes-sigs/kubebuilder#2596])

Only to get the changes from what we currently use in SDK from kubebuilder.

- Ensure that scaffolds begin to be performed with a restrive Security Context
- Be able to do specific follow up PR so we can discuss properly how we will consume the new additions on Kubebuilder
- Make eaiser the process to keep SDK updated, otherwise, it is hard to properly review if we wait to do a big PR with all.
- Ensure that SDK features can work with the latest changes performed in Kubebuilder

**Extra info**
You can check the latest changes on Kubebuilder by looking: kubernetes-sigs/kubebuilder@v3.4.1...master

Note that SDK users Kubebuilder as a LIB. So that all that is currently important is updated by default when we bump. However, all that was added to Kubebuilder but still not imported/used in SDK will not be added here. Therefore we can do follow up PRs to get the additions. Why? we have significant changes and additions for example adding the support for the phase 2 plugin, but if we try to do it all in the same PR it will be very hard to get properly reviewed.
camilamacedo86 pushed a commit to camilamacedo86/operator-sdk that referenced this pull request May 29, 2022
Signed-off-by: Camila Macedo <cmacedo@redhat.com>

- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1): applying restrictive SCC for all containers scaffolded by the tool (`k8s versions < 1.19 will no longer work with`). ([More info](kubernetes-sigs/kubebuilder#2700))
- For Golang-based language (go/v2) fix the issue introduced by removing the GO111MODULE=on from Dockerfile. ([More info](kubernetes-sigs/kubebuilder#2678))
- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1 language (go/v3), add a new comment with the option `leaderElectionReleaseOnCancel` ((More info)[kubernetes-sigs/kubebuilder#2596])

Only to get the changes from what we currently use in SDK from kubebuilder.

- Ensure that scaffolds begin to be performed with a restrive Security Context
- Be able to do specific follow up PR so we can discuss properly how we will consume the new additions on Kubebuilder
- Make eaiser the process to keep SDK updated, otherwise, it is hard to properly review if we wait to do a big PR with all.
- Ensure that SDK features can work with the latest changes performed in Kubebuilder

**Extra info**
You can check the latest changes on Kubebuilder by looking: kubernetes-sigs/kubebuilder@v3.4.1...master

Note that SDK users Kubebuilder as a LIB. So that all that is currently important is updated by default when we bump. However, all that was added to Kubebuilder but still not imported/used in SDK will not be added here. Therefore we can do follow up PRs to get the additions. Why? we have significant changes and additions for example adding the support for the phase 2 plugin, but if we try to do it all in the same PR it will be very hard to get properly reviewed.
camilamacedo86 pushed a commit to camilamacedo86/operator-sdk that referenced this pull request Jun 7, 2022
Signed-off-by: Camila Macedo <cmacedo@redhat.com>

- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1): applying restrictive SCC for all containers scaffolded by the tool (`k8s versions < 1.19 will no longer work with`). ([More info](kubernetes-sigs/kubebuilder#2700))
- For Golang-based language (go/v2) fix the issue introduced by removing the GO111MODULE=on from Dockerfile. ([More info](kubernetes-sigs/kubebuilder#2678))
- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1 language (go/v3), add a new comment with the option `leaderElectionReleaseOnCancel` ((More info)[kubernetes-sigs/kubebuilder#2596])

Only to get the changes from what we currently use in SDK from kubebuilder.

- Ensure that scaffolds begin to be performed with a restrive Security Context
- Be able to do specific follow up PR so we can discuss properly how we will consume the new additions on Kubebuilder
- Make eaiser the process to keep SDK updated, otherwise, it is hard to properly review if we wait to do a big PR with all.
- Ensure that SDK features can work with the latest changes performed in Kubebuilder

**Extra info**
You can check the latest changes on Kubebuilder by looking: kubernetes-sigs/kubebuilder@v3.4.1...master

Note that SDK users Kubebuilder as a LIB. So that all that is currently important is updated by default when we bump. However, all that was added to Kubebuilder but still not imported/used in SDK will not be added here. Therefore we can do follow up PRs to get the additions. Why? we have significant changes and additions for example adding the support for the phase 2 plugin, but if we try to do it all in the same PR it will be very hard to get properly reviewed.
camilamacedo86 pushed a commit to camilamacedo86/operator-sdk that referenced this pull request Jun 8, 2022
Signed-off-by: Camila Macedo <cmacedo@redhat.com>

- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1): applying restrictive SCC for all containers scaffolded by the tool (k8s versions < 1.19 will no longer work with).
- For Golang-based language (go/v2) fix the issue introduced by removing the GO111MODULE=on from Dockerfile. ([More info](kubernetes-sigs/kubebuilder#2678))
- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1 language (go/v3), add a new comment with the option `leaderElectionReleaseOnCancel` ((More info)[kubernetes-sigs/kubebuilder#2596])

Only to get the changes from what we currently use in SDK from kubebuilder.

- Ensure that scaffolds begin to be performed with a restrive Security Context
- Be able to do specific follow up PR so we can discuss properly how we will consume the new additions on Kubebuilder
- Make eaiser the process to keep SDK updated, otherwise, it is hard to properly review if we wait to do a big PR with all.
- Ensure that SDK features can work with the latest changes performed in Kubebuilder

**Extra info**
You can check the latest changes on Kubebuilder by looking: kubernetes-sigs/kubebuilder@v3.4.1...master

Note that SDK users Kubebuilder as a LIB. So that all that is currently important is updated by default when we bump. However, all that was added to Kubebuilder but still not imported/used in SDK will not be added here. Therefore we can do follow up PRs to get the additions. Why? we have significant changes and additions for example adding the support for the phase 2 plugin, but if we try to do it all in the same PR it will be very hard to get properly reviewed.

Co-authored-by: Varsha <varshaprasad96@gmail.com>
camilamacedo86 pushed a commit to camilamacedo86/operator-sdk that referenced this pull request Jun 8, 2022
Signed-off-by: Camila Macedo <cmacedo@redhat.com>

- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1): applying restrictive SCC for all containers scaffolded by the tool (k8s versions < 1.19 will no longer work with).
- For Golang-based language (go/v2) fix the issue introduced by removing the GO111MODULE=on from Dockerfile. ([More info](kubernetes-sigs/kubebuilder#2678))
- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1 language (go/v3), add a new comment with the option `leaderElectionReleaseOnCancel` ((More info)[kubernetes-sigs/kubebuilder#2596])

Only to get the changes from what we currently use in SDK from kubebuilder.

- Ensure that scaffolds begin to be performed with a restrive Security Context
- Be able to do specific follow up PR so we can discuss properly how we will consume the new additions on Kubebuilder
- Make eaiser the process to keep SDK updated, otherwise, it is hard to properly review if we wait to do a big PR with all.
- Ensure that SDK features can work with the latest changes performed in Kubebuilder

**Extra info**
You can check the latest changes on Kubebuilder by looking: kubernetes-sigs/kubebuilder@v3.4.1...master

Note that SDK users Kubebuilder as a LIB. So that all that is currently important is updated by default when we bump. However, all that was added to Kubebuilder but still not imported/used in SDK will not be added here. Therefore we can do follow up PRs to get the additions. Why? we have significant changes and additions for example adding the support for the phase 2 plugin, but if we try to do it all in the same PR it will be very hard to get properly reviewed.

Co-authored-by: Varsha <varshaprasad96@gmail.com>
camilamacedo86 added a commit to operator-framework/operator-sdk that referenced this pull request Jun 8, 2022
* Upgrade Kubebuilder dep from 3.4.1 to the commit 800fdeec6e5c

Signed-off-by: Camila Macedo <cmacedo@redhat.com>

- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1): applying restrictive SCC for all containers scaffolded by the tool (k8s versions < 1.19 will no longer work with).
- For Golang-based language (go/v2) fix the issue introduced by removing the GO111MODULE=on from Dockerfile. ([More info](kubernetes-sigs/kubebuilder#2678))
- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1 language (go/v3), add a new comment with the option `leaderElectionReleaseOnCancel` ((More info)[kubernetes-sigs/kubebuilder#2596])

Only to get the changes from what we currently use in SDK from kubebuilder.

- Ensure that scaffolds begin to be performed with a restrive Security Context
- Be able to do specific follow up PR so we can discuss properly how we will consume the new additions on Kubebuilder
- Make eaiser the process to keep SDK updated, otherwise, it is hard to properly review if we wait to do a big PR with all.
- Ensure that SDK features can work with the latest changes performed in Kubebuilder

**Extra info**
You can check the latest changes on Kubebuilder by looking: kubernetes-sigs/kubebuilder@v3.4.1...master

Note that SDK users Kubebuilder as a LIB. So that all that is currently important is updated by default when we bump. However, all that was added to Kubebuilder but still not imported/used in SDK will not be added here. Therefore we can do follow up PRs to get the additions. Why? we have significant changes and additions for example adding the support for the phase 2 plugin, but if we try to do it all in the same PR it will be very hard to get properly reviewed.

Co-authored-by: Varsha <varshaprasad96@gmail.com>

* trying to fix ansible lint

Co-authored-by: Varsha <varshaprasad96@gmail.com>
VenkatRamaraju pushed a commit to VenkatRamaraju/operator-sdk that referenced this pull request Jun 9, 2022
…or-framework#5814)

* Upgrade Kubebuilder dep from 3.4.1 to the commit 800fdeec6e5c

Signed-off-by: Camila Macedo <cmacedo@redhat.com>

- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1): applying restrictive SCC for all containers scaffolded by the tool (k8s versions < 1.19 will no longer work with).
- For Golang-based language (go/v2) fix the issue introduced by removing the GO111MODULE=on from Dockerfile. ([More info](kubernetes-sigs/kubebuilder#2678))
- For Golang/Ansible/Helm language-based operators (go/v3, ansible/v1 and helm/v1 language (go/v3), add a new comment with the option `leaderElectionReleaseOnCancel` ((More info)[kubernetes-sigs/kubebuilder#2596])

Only to get the changes from what we currently use in SDK from kubebuilder.

- Ensure that scaffolds begin to be performed with a restrive Security Context
- Be able to do specific follow up PR so we can discuss properly how we will consume the new additions on Kubebuilder
- Make eaiser the process to keep SDK updated, otherwise, it is hard to properly review if we wait to do a big PR with all.
- Ensure that SDK features can work with the latest changes performed in Kubebuilder

**Extra info**
You can check the latest changes on Kubebuilder by looking: kubernetes-sigs/kubebuilder@v3.4.1...master

Note that SDK users Kubebuilder as a LIB. So that all that is currently important is updated by default when we bump. However, all that was added to Kubebuilder but still not imported/used in SDK will not be added here. Therefore we can do follow up PRs to get the additions. Why? we have significant changes and additions for example adding the support for the phase 2 plugin, but if we try to do it all in the same PR it will be very hard to get properly reviewed.

Co-authored-by: Varsha <varshaprasad96@gmail.com>

* trying to fix ansible lint

Co-authored-by: Varsha <varshaprasad96@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set LeaderElectionReleaseOnCancel to the scaffolded main.go
5 participants