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

Add Mofed termination grace period param #443

Merged
merged 1 commit into from
Jan 4, 2023

Conversation

rollandf
Copy link
Member

@rollandf rollandf commented Jan 3, 2023

Adding to NicClusterPolicy CRD the possibility to specify the termination grace period for the Mofed container. The default is 300 seconds.

It is also configurable in Helm values file.

Signed-off-by: Fred Rolland frolland@nvidia.com

@rollandf rollandf force-pushed the mofed-termination branch 2 times, most recently from db07f6e to bb6ea4c Compare January 3, 2023 13:52
Copy link
Collaborator

@e0ne e0ne left a comment

Choose a reason for hiding this comment

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

LTGM once CI passed

@e0ne e0ne mentioned this pull request Jan 3, 2023
24 tasks
// +optional
// +kubebuilder:default:=300
// +kubebuilder:validation:Minimum:=0
TerminationGracePeriodSeconds int `json:"terminationGracePeriodSeconds,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/kubernetes/api/blob/92fe7a3ca2f4bd72b8f6faeab90fc75f4d1aab27/core/v1/types.go#L3173

lets make it the same as in k8s (i.e int64) WDYT ?

also do you see a benefit of having it a pointer vs non-pointer ?
for me, if we have default i think it fine to go with non-pointer type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

lets make it the same as in k8s (i.e int64) WDYT ?

Good catch, I agree

Copy link
Member Author

Choose a reason for hiding this comment

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

updated to int64

---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.4.1
controller-gen.kubebuilder.io/version: v0.10.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

we really need to have some makefile targets to get a pre-defined version of controller-gen/operator-sdk so all generate with same version

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is a bug filed: #444

Copy link
Member Author

Choose a reason for hiding this comment

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

The version is defined here: https://github.com/Mellanox/network-operator/blob/master/Makefile#L248

The problem is that if you have an older version, it will not be updated unless you delete the older version from ./bin directory.

Copy link
Collaborator

@adrianchiris adrianchiris Jan 3, 2023

Choose a reason for hiding this comment

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

ack so it just making sure CRD are updated using up2date version. in that case i dont think we need the bug.

we could technically add a CI step to regenerated crds and make sure no diffs or simillar

Adding to NicClusterPolicy CRD the possibility to specify
the termination grace period for the Mofed container.
The default is 300 seconds.

It is also configurable in Helm values file.

Signed-off-by: Fred Rolland <frolland@nvidia.com>
@e0ne
Copy link
Collaborator

e0ne commented Jan 4, 2023

/retest_helm_ci

@e0ne
Copy link
Collaborator

e0ne commented Jan 4, 2023

/retest-nic_operator_helm

@e0ne e0ne merged commit 7732e96 into Mellanox:master Jan 4, 2023
@rollandf rollandf deleted the mofed-termination branch February 28, 2024 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants