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

🐛 fixes services for EKS clusters #3343

Merged
merged 1 commit into from Mar 30, 2022
Merged

🐛 fixes services for EKS clusters #3343

merged 1 commit into from Mar 30, 2022

Conversation

faiq
Copy link
Contributor

@faiq faiq commented Mar 23, 2022 •
edited

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR creates a failing test for #3329 as well as extending the existing test for non managed clusters to ensure the LB for service is actually backed by instances.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Release note:

changes AWSCloudProvider tag for ec2 instances to be KubernetesClusterName() instead of ClusterName() to support load balancer services for EKS clusters

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 23, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @faiq. 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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 23, 2022
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 23, 2022
@dlipovetsky
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 23, 2022
@dlipovetsky
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 23, 2022
@dlipovetsky
Copy link
Contributor

(Setting same priority as #3329)

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority labels Mar 23, 2022
@faiq faiq force-pushed the main branch 3 times, most recently from c7c3069 to 7ef9d22 Compare March 23, 2022 23:30
@faiq
Copy link
Contributor Author

faiq commented Mar 24, 2022

/retest

@shivi28
Copy link
Contributor

shivi28 commented Mar 24, 2022

/test ?

@k8s-ci-robot
Copy link
Contributor

@shivi28: The following commands are available to trigger required jobs:

  • /test pull-cluster-api-provider-aws-build
  • /test pull-cluster-api-provider-aws-test
  • /test pull-cluster-api-provider-aws-verify

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-provider-aws-apidiff-main
  • /test pull-cluster-api-provider-aws-e2e
  • /test pull-cluster-api-provider-aws-e2e-blocking
  • /test pull-cluster-api-provider-aws-e2e-conformance
  • /test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts
  • /test pull-cluster-api-provider-aws-e2e-eks

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-provider-aws-apidiff-main
  • pull-cluster-api-provider-aws-build
  • pull-cluster-api-provider-aws-test
  • pull-cluster-api-provider-aws-verify

In response to this:

/test ?

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.

@shivi28
Copy link
Contributor

shivi28 commented Mar 24, 2022

/test pull-cluster-api-provider-aws-e2e

@pydctw
Copy link
Contributor

pydctw commented Mar 24, 2022

For EKS lb creation and verification
/test pull-cluster-api-provider-aws-e2e-eks

@faiq
Copy link
Contributor Author

faiq commented Mar 28, 2022

/test pull-cluster-api-provider-aws-e2e-eks

@faiq faiq changed the title :test: check instances for load balancers and add LB test to EKS 🐛 fixes services for EKS clusters Mar 28, 2022
@faiq
Copy link
Contributor Author

faiq commented Mar 28, 2022

/retest

@faiq
Copy link
Contributor Author

faiq commented Mar 28, 2022

/test pull-cluster-api-provider-aws-e2e-eks

1 similar comment
@faiq
Copy link
Contributor Author

faiq commented Mar 28, 2022

/test pull-cluster-api-provider-aws-e2e-eks

@faiq
Copy link
Contributor Author

faiq commented Mar 28, 2022

/test pull-cluster-api-provider-aws-e2e-eks

@sedefsavas
Copy link
Contributor

Since a shared method (VerifyElbExists) is changed:

/test pull-cluster-api-provider-aws-e2e

@sedefsavas
Copy link
Contributor

Since this a bug fix, +1 to backport onto release-1.3

We will make v1.4 release soon, as its been 2 months since the last release and we have some feat work in.
@dlipovetsky is there any urgency for this bug to make a patch release to v1.2?

@richardcase
Copy link
Member

This looks good to me.

@faiq - could you squash your commits?

@sedefsavas
Copy link
Contributor

The additional deployment added here does not give any signal as is.

Expect(len(elbsOutput.LoadBalancerDescriptions[0].Instances)).Should(BeNumerically(">=", 1))

This is true for even if we don't have a deployment that matches the Service. The instances attached to the LB are just Out of Service if there are no pods in the instance.

You can either add a check that shows the instances with the pods are in service in the LB,
or can remove the deployment all together.

@faiq
Copy link
Contributor Author

faiq commented Mar 29, 2022

The additional deployment added here does not give any signal as is.

Expect(len(elbsOutput.LoadBalancerDescriptions[0].Instances)).Should(BeNumerically(">=", 1))

This is true for even if we don't have a deployment that matches the Service. The instances attached to the LB are just Out of Service if there are no pods in the instance.
You can either add a check that shows the instances with the pods are in service in the LB,
or can remove the deployment all together.

I added the change because I was asked to here #3343 (comment)

It makes the test code flow a little bit better, but I agree we don't need it to test the functionality.

If you're still ok with having it removed, just let me know and I'll do it

@sedefsavas
Copy link
Contributor

I added the change because I was asked to here #3343 (comment)

Don't have a strong preference, we just don't need the code if we are not using it.
@pydctw WDYT?

@sedefsavas
Copy link
Contributor

Also there is a way to check if the instances are in service in the ELBs with (c *ELB) DescribeInstanceHealth(input *DescribeInstanceHealthInput).

If this check is added to verifyELB, then adding a deployment will make sense IMO.

@faiq
Copy link
Contributor Author

faiq commented Mar 29, 2022

Also there is a way to check if the instances are in service in the ELBs with (c *ELB) DescribeInstanceHealth(input *DescribeInstanceHealthInput).
If this check is added to verifyELB, then adding a deployment will make sense IMO.

I'm a little confused on your ask here. To me it sounds like you want me to:

  1. get the node the nginx deployment is running on
  2. check to see if that DescribeInstanceHealth to see if the node is in the list

i would prefer to simply remove the deployment

@pydctw
Copy link
Contributor

pydctw commented Mar 30, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 30, 2022
@sedefsavas
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sedefsavas

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

The pull request process is described here

Needs approval from an approver in each of these files:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2022
@k8s-ci-robot k8s-ci-robot merged commit d1fcc96 into kubernetes-sigs:main Mar 30, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.x milestone Mar 30, 2022
@faiq
Copy link
Contributor Author

faiq commented Mar 31, 2022

/test pull-cluster-api-provider-aws-e2e-eks

@sedefsavas sedefsavas mentioned this pull request Apr 4, 2022
Merged
4 tasks
k8s-ci-robot added a commit that referenced this pull request Apr 4, 2022
@k8s-ci-robot
Merge pull request #3379 from sedefsavas/revert-3343
831a0ab 
Revert #3343 e2e test changes
@faiq faiq mentioned this pull request Jun 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pydctw pydctw pydctw left review comments

@dthorsen dthorsen Awaiting requested review from dthorsen

@shivi28 shivi28 Awaiting requested review from shivi28

Assignees

@pydctw pydctw

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. kind/bug Categorizes issue or PR as related to a bug. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@faiq @k8s-ci-robot @dlipovetsky @shivi28 @pydctw @sedefsavas @richardcase