-
Notifications
You must be signed in to change notification settings - Fork 37
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
Implement Non-Strict Affinity and Non-Strict Anti-Affinity Support #330
Implement Non-Strict Affinity and Non-Strict Anti-Affinity Support #330
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-cloudstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: FarnazBGH The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @FarnazBGH! |
Hi @FarnazBGH. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
9158f7e
to
b918dbc
Compare
b918dbc
to
ce0ec25
Compare
/uncc @chrisdoherty4 |
/ok-to-test |
/run-e2e -c 4.18 |
@rohityadavcloud a jenkins job has been kicked to run test with following paramaters:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR @FarnazBGH!
How would this behave if the user is running an older version of CloudStack that doesn't support this feature?
Test Results : (tid-355)
|
In our environment all the CloudStack are upgraded to the latest version so non-strict affinities are available, in response to your question I created an extra-test affinity (assume there is affinity in the later version which is test-anti) and revised the code with test affinity which is not available in our system which was (TestAntiAffinity = "test-anti"):
Now during the creation of the cluster when I set the affinity to the value of"test-anti", I will get the below error:
which means for the lower version of cloudstack they will get similar errors for the non-strict affinities.
|
LGTM but waiting for input from other maintainers as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some small comments.
Question from my side to the other maintainers would be whether its a good idea to apply this change to v1beta1 and v1beta2, or if it doesn't really matter because the type of value is not changing, just the allowed values.
And a general suggestion for this Type field from my side would be to consider using the following kubebuilder validation so the CRD already defines the allowed values:
In cloudstackaffinitygroup_types.go
:
type CloudStackAffinityGroupSpec struct {
// Mutually exclusive parameter with AffinityGroupIDs.
// Can be "host affinity" or "host anti-affinity". Will create an affinity group per machine set.
// +kubebuilder:validation:Enum=host affinity;host anti-affinity;non-strict host affinity;non-strict host anti-affinity
// +optional
Type string `json:"type,omitempty"`
In cloudstackmachine_types.go
:
// Mutually exclusive parameter with AffinityGroupIDs.
// Defaults to `no`. Can be `pro` or `anti`. Will create an affinity group per machine set.
// +kubebuilder:validation:Enum=no;pro;anti;soft-pro;soft-anti
// +optional
Affinity string `json:"affinity,omitempty"`
Another question from my end is why we use two different formats for the affinity types (host affinity
vs pro
f.e.), but this might be something we can deal with in a seperate PR.
ce0ec25
to
148da67
Compare
/run-e2e -c 4.18 |
9cdf7eb
to
3a9f88c
Compare
3a9f88c
to
1e04f47
Compare
Sorry for the delayed response. The comments and documentation are added. In the next few days, I will also work on checking and testing the CloudStack version for the soft-pro and soft-anti. |
Test Results : (tid-470)
|
Test Results : (tid-472)
|
…r versions lowerThan 4.18
ed7d165
to
2f0e962
Compare
@@ -73,6 +73,22 @@ func AffinityGroupSpec(ctx context.Context, inputGetter func() CommonSpecInput) | |||
affinityIds = executeTest(ctx, input, namespace, specName, clusterResources, "anti") | |||
}) | |||
|
|||
It("Should have host affinity group when affinity is soft-pro", func() { | |||
cloudStackVersion := input.E2EConfig.GetVariable("CLOUDSTACK_VERSION") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this dynamic and fetch the version from CloudStack itself?
The below code is not merged, but maybe something like this?
AffinityGroupType = "host affinity" | ||
AntiAffinityGroupType = "host anti-affinity" | ||
AffinityGroupType = "host affinity" | ||
SoftAntiAffinityGroupType = "non-strict anti-affinity" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SoftAntiAffinityGroupType = "non-strict anti-affinity" | |
SoftAntiAffinityGroupType = "non-strict host anti-affinity" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FarnazBGH This needs to be fixed.
@FarnazBGH I will be cutting the RC on Monday. If possible, please address/resolve the comments before then. Else we will have to move this to the next release. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
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-sigs/prow repository. |
Description of changes:
This PR integrates non-strict Anti-affinity and non-strict Affinity types into CloudStackAffinityGroup. With the latest CloudStack version 4.18, we now have the capability to set these flexible affinity rules. This enhancement allows for more adaptable and efficient resource management, particularly beneficial for the configuration of worker nodes.
Testing performed:
CloudStackMachineTemplate
for the worker machines.Below is an example configuration showcasing the use of a non-strict anti-affinity type in
CloudStackMachineTemplate
:Requirements:
CloudStack Version: This feature requires CloudStack version 4.18.1 or higher.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.