Implement Non-Strict Affinity and Non-Strict Anti-Affinity Support

FarnazBGH commented 11 months ago

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:

Make Test

Creating cluster with similar CloudStackMachineTemplate for the worker machines. Below is an example configuration showcasing the use of a non-strict anti-affinity type in CloudStackMachineTemplate:

apiVersion: infrastructure.cluster.x-k8s.io/v1beta2
kind: CloudStackMachineTemplate
metadata:
name: example-worker-0
spec:
template:
  spec:
    affinity: soft-anti
    offering:
      name: example-offering
    template:
      name: example-template

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.

linux-foundation-easycla[bot] commented 11 months ago

The committers listed above are authorized under a signed CLA.

:white_check_mark: login: FarnazBGH / name: Farnaz (1e04f47cfb5d2edc1c57ef134ee505599d40fcff, 148da67b2a455c46b8fe239a974fff3781f49d4d, 2f0e962d9ca706807fa7674a4c65bebe1eedc9d6)

netlify[bot] commented 11 months ago

Deploy Preview for kubernetes-sigs-cluster-api-cloudstack ready!

Name	Link
Latest commit	2f0e962d9ca706807fa7674a4c65bebe1eedc9d6
Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-cloudstack/deploys/665f0bce4018fe00081d3780
Deploy Preview	https://deploy-preview-330--kubernetes-sigs-cluster-api-cloudstack.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

k8s-ci-robot commented 11 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: FarnazBGH Once this PR has been reviewed and has the lgtm label, please assign davidjumani for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-cloudstack/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

k8s-ci-robot commented 11 months ago

Welcome @FarnazBGH!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-cloudstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-cloudstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

k8s-ci-robot commented 11 months ago

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

chrisdoherty4 commented 11 months ago

/uncc @chrisdoherty4 /cc @g-gaston

rohityadavcloud commented 11 months ago

/ok-to-test

rohityadavcloud commented 11 months ago

/run-e2e -c 4.18

blueorangutan commented 11 months ago

@rohityadavcloud a jenkins job has been kicked to run test with following paramaters:

kubernetes version: 1.27.2
CloudStack version: 4.18
hypervisor: kvm
template: ubuntu-2004-kube
Kubernetes upgrade from: 1.26.5 to 1.27.2

blueorangutan commented 11 months ago

Test Results : (tid-355) Environment: kvm Rocky8(x3), Advanced Networking with Management Server Rocky8 Kubernetes Version: v1.27.2 Kubernetes Version upgrade from: v1.26.5 Kubernetes Version upgrade to: v1.27.2 CloudStack Version: 4.18 Template: ubuntu-2004-kube E2E Test Run Logs: https://github.com/blueorangutan/capc-prs/releases/download/capc-pr-ci-cd/capc-e2e-artifacts-pr330-sl-355.zip

[PASS] When testing affinity group Should have host affinity group when affinity is anti
[PASS] When testing machine remediation Should replace a machine when it is destroyed
[PASS] When testing horizontal scale out/in [TC17][TC18][TC20][TC21] Should successfully scale machine replicas up and down horizontally
[PASS] When the specified resource does not exist Should fail due to the specified account is not found [TC4a]
[PASS] When the specified resource does not exist Should fail due to the specified domain is not found [TC4b]
[PASS] When the specified resource does not exist Should fail due to the specified control plane offering is not found [TC7]
[PASS] When the specified resource does not exist Should fail due to the specified template is not found [TC6]
[PASS] When the specified resource does not exist Should fail due to the specified zone is not found [TC3]
[PASS] When the specified resource does not exist Should fail due to the specified disk offering is not found
[PASS] When the specified resource does not exist Should fail due to the compute resources are not sufficient for the specified offering [TC8]
[PASS] When the specified resource does not exist Should fail due to the specified disk offer is not customized but the disk size is specified
[PASS] When the specified resource does not exist Should fail due to the specified disk offer is customized but the disk size is not specified
[PASS] When the specified resource does not exist Should fail due to the public IP can not be found
[PASS] When the specified resource does not exist When starting with a healthy cluster Should fail to upgrade worker machine due to insufficient compute resources
[PASS] When the specified resource does not exist When starting with a healthy cluster Should fail to upgrade control plane machine due to insufficient compute resources
[PASS] When testing subdomain Should create a cluster in a subdomain
[PASS] When testing app deployment to the workload cluster [TC1][PR-Blocking] Should be able to download an HTML from the app deployed to the workload cluster
[PASS] When testing K8S conformance [Conformance] Should create a workload cluster and run kubetest
[PASS] When testing MachineDeployment rolling upgrades Should successfully upgrade Machines upon changes in relevant MachineDeployment fields
[PASS] When testing with custom disk offering Should successfully create a cluster with a custom disk offering
[PASS] When testing resource cleanup Should create a new network when the specified network does not exist
[PASS] When testing node drain timeout A node should be forcefully removed if it cannot be drained in time
[PASS] with two clusters should successfully add and remove a second cluster without breaking the first cluster
[PASS] When testing with disk offering Should successfully create a cluster with disk offering

Summarizing 7 Failures:

[Fail] When testing Kubernetes version upgrades [It] Should successfully upgrade kubernetes versions when there is a change in relevant fields 
/root/go/pkg/mod/sigs.k8s.io/cluster-api/test@v1.2.12/framework/machinedeployment_helpers.go:129

[Fail] When testing affinity group [It] Should have host affinity group when affinity is pro 
/jenkins/workspace/capc-e2e-new/test/e2e/common.go:331

[Fail] When testing affinity group [It] Should have host affinity group when affinity is soft-pro 
/root/go/pkg/mod/sigs.k8s.io/cluster-api/test@v1.2.12/framework/clusterctl/client.go:248

[Fail] When testing affinity group [It] Should have host affinity group when affinity is soft-anti 
/root/go/pkg/mod/sigs.k8s.io/cluster-api/test@v1.2.12/framework/clusterctl/client.go:248

[Fail] When testing app deployment to the workload cluster with network interruption [ToxiProxy] [BeforeEach] Should be able to create a cluster despite a network interruption during that process 
/jenkins/workspace/capc-e2e-new/test/e2e/toxiproxy/toxiProxy.go:203

[Fail] When testing app deployment to the workload cluster with slow network [ToxiProxy] [BeforeEach] Should be able to download an HTML from the app deployed to the workload cluster 
/jenkins/workspace/capc-e2e-new/test/e2e/toxiproxy/toxiProxy.go:203

[Fail] When testing multiple CPs in a shared network with kubevip [It] Should successfully create a cluster with multiple CPs in a shared network 
/root/go/pkg/mod/sigs.k8s.io/cluster-api/test@v1.2.12/framework/cluster_helpers.go:143

Ran 30 of 31 Specs in 9834.870 seconds
FAIL! -- 23 Passed | 7 Failed | 0 Pending | 1 Skipped
--- FAIL: TestE2E (9834.88s)
FAIL

FarnazBGH commented 10 months ago

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?

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"):

const (
    // The presence of a finalizer prevents CAPI from deleting the corresponding CAPI data.
    MachineFinalizer = "cloudstackmachine.infrastructure.cluster.x-k8s.io"
    ProAffinity      = "pro"
    AntiAffinity     = "anti"
    SoftProAffinity  = "soft-pro"
    SoftAntiAffinity = "soft-anti"
    TestAntiAffinity = "test-anti"
    NoAffinity       = "no"
)

Now during the creation of the cluster when I set the affinity to the value of"test-anti", I will get the below error:

E0109 09:53:14.109649       1 controller.go:324]  "msg"="Reconciler error" "error"="CloudStack API error 431 (CSExceptionErrorCode: 4350): Unable to create affinity group, invalid affinity group type: test host anti-affinity. Valid values are non-strict host affinity, host affinity,host anti-affinity,ExplicitDedication,non-strict host anti-affinity"

which means for the lower version of cloudstack they will get similar errors for the non-strict affinities. For example for " non-strict host anti-affinity" something similar to:

CloudStack API error 431 (CSExceptionErrorCode: 4350): Unable to create affinity group, invalid affinity group type: non-strict host anti-affinity. Valid values are host affinity, host anti-affinity, ExplicitDedication"

davidjumani commented 10 months ago

LGTM but waiting for input from other maintainers as well. @g-gaston If this is tied to a specific version of cloudstack, the compatibility matrix can be updated accordingly when a release with this feature is cut so users are aware of this requirement

rohityadavcloud commented 10 months ago

/run-e2e -c 4.18

g-gaston commented 10 months ago

Apologies for the delay in the review, I'll get to this :)

g-gaston commented 9 months ago

LGTM but waiting for input from other maintainers as well. @g-gaston If this is tied to a specific version of cloudstack, the compatibility matrix can be updated accordingly when a release with this feature is cut so users are aware of this requirement

Totally. However, I would prefer not to drop support for older versions (for now) just to support this small feature.

I think this brings us to the "partial support" territory: not all the features work in all supported versions. I think as long as we document clearly what we support and what we not and what is the expected behavior (if you use this feature in older versions, validations will pass but you'll get a reconciler error), this is a reasonable approach.

Opinions?

@FarnazBGH could you add a line to the go comment saying that these new values are only supported in CS 4.18.1 or higher? And we need to find a place to document this in the book/docs. Do you mind taking a look and proposing a change for it?

vishesh92 commented 6 months ago

I think this brings us to the "partial support" territory: not all the features work in all supported versions. I think as long as we document clearly what we support and what we not and what is the expected behavior (if you use this feature in older versions, validations will pass but you'll get a reconciler error), this is a reasonable approach.

Opinions?

Agree with @g-gaston . If the affinity group is not supported, reconciler error should be fine.

vishesh92 commented 6 months ago

@FarnazBGH could you add a line to the go comment saying that these new values are only supported in CS 4.18.1 or higher? And we need to find a place to document this in the book/docs. Do you mind taking a look and proposing a change for it?

@FarnazBGH Can you update the documentation about the new affinity group types and add a note about ACS version compatability in https://github.com/kubernetes-sigs/cluster-api-provider-cloudstack/blob/main/docs/book/src/clustercloudstack/configuration.md#affinity-groups?

vishesh92 commented 6 months ago

@FarnazBGH I am planning for a release next week. Do you think this PR will be ready by then?

FarnazBGH commented 6 months ago

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.

blueorangutan commented 6 months ago

Test Results : (tid-470) Environment: kvm Rocky8(x3), Advanced Networking with Management Server Rocky8 Kubernetes Version: v1.27.2 Kubernetes Version upgrade from: v1.26.5 Kubernetes Version upgrade to: v1.27.2 CloudStack Version: 4.19 Template: ubuntu-2004-kube E2E Test Run Logs: https://github.com/blueorangutan/capc-prs/releases/download/capc-pr-ci-cd/capc-e2e-artifacts-pr330-sl-470.zip


Summarizing 6 Failures:
 [FAIL] When testing affinity group [It] Should have host affinity group when affinity is pro
 /jenkins/workspace/capc-e2e-new/test/e2e/common.go:348
 [FAIL] When testing affinity group [It] Should have host affinity group when affinity is soft-pro
 /root/go/pkg/mod/sigs.k8s.io/cluster-api/test@v1.4.8/framework/clusterctl/client.go:292
 [FAIL] When testing affinity group [It] Should have host affinity group when affinity is soft-anti
 /root/go/pkg/mod/sigs.k8s.io/cluster-api/test@v1.4.8/framework/clusterctl/client.go:292
 [FAIL] When testing MachineDeployment rolling upgrades [It] Should successfully upgrade Machines upon changes in relevant MachineDeployment fields
 /root/go/pkg/mod/sigs.k8s.io/cluster-api/test@v1.4.8/framework/machinedeployment_helpers.go:329
 [FAIL] When testing project [AfterEach] Should create a cluster in a project
 /jenkins/workspace/capc-e2e-new/test/e2e/project.go:103
 [FAIL] with two clusters [It] should successfully add and remove a second cluster without breaking the first cluster
 /root/go/pkg/mod/sigs.k8s.io/cluster-api/test@v1.4.8/framework/clusterctl/clusterctl_helpers.go:345

Ran 31 of 32 Specs in 10470.880 seconds
FAIL! -- 25 Passed | 6 Failed | 0 Pending | 1 Skipped
--- FAIL: TestE2E (10470.88s)
FAIL

blueorangutan commented 6 months ago

Test Results : (tid-472) Environment: kvm Rocky8(x3), Advanced Networking with Management Server Rocky8 Kubernetes Version: v1.27.2 Kubernetes Version upgrade from: v1.26.5 Kubernetes Version upgrade to: v1.27.2 CloudStack Version: 4.19 Template: ubuntu-2004-kube E2E Test Run Logs: https://github.com/blueorangutan/capc-prs/releases/download/capc-pr-ci-cd/capc-e2e-artifacts-pr330-sl-472.zip


Summarizing 6 Failures:
 [FAIL] When testing project [AfterEach] Should create a cluster in a project
 /jenkins/workspace/capc-e2e-new/test/e2e/project.go:103
 [FAIL] When testing affinity group [It] Should have host affinity group when affinity is pro
 /jenkins/workspace/capc-e2e-new/test/e2e/common.go:348
 [FAIL] When testing affinity group [It] Should have host affinity group when affinity is soft-pro
 /root/go/pkg/mod/sigs.k8s.io/cluster-api/test@v1.4.8/framework/clusterctl/client.go:292
 [FAIL] When testing affinity group [It] Should have host affinity group when affinity is soft-anti
 /root/go/pkg/mod/sigs.k8s.io/cluster-api/test@v1.4.8/framework/clusterctl/client.go:292
 [FAIL] When testing machine remediation [It] Should replace a machine when it is destroyed
 /jenkins/workspace/capc-e2e-new/test/e2e/common.go:495
 [TIMEDOUT] When testing with disk offering [AfterEach] Should successfully create a cluster with disk offering
 /jenkins/workspace/capc-e2e-new/test/e2e/disk_offering.go:89

Ran 31 of 32 Specs in 10786.159 seconds
FAIL! - Suite Timeout Elapsed -- 25 Passed | 6 Failed | 0 Pending | 1 Skipped
--- FAIL: TestE2E (10786.16s)
FAIL

vishesh92 commented 5 months ago

@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.

k8s-triage-robot commented 1 month ago

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 4 weeks ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

kubernetes-sigs / cluster-api-provider-cloudstack