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

enable node events when instance type is not supported #218

Merged
merged 1 commit into from
May 8, 2023

Conversation

haouc
Copy link
Contributor

@haouc haouc commented May 5, 2023

Issue #, if available:

Description of changes:
Since this controller runs in EKS control plane, if the instance type is not supported, the error message is not available for users. To make users' debugging easier and better visibility, we should expose some of critical failures that can be mitigated from user side. Events are common use for this purpose. This change propose sending node event when

  • new instance types are not included to support for this controller (Linux SGP feature or Windows)
  • instance types are in the supported types list but doesn't support trunk interface (SGP feature)

Other changes:

  • refactoring how passing client to node/instance. Since node is k8s object and also carry AWS instance, I think it is making sense to add k8s/ec2 clients into node struct instead of need pass them to functions when calling node resources.
  • updating unit tests

Tests:
The unsupported and mocked missing instance types were used to test the events

case 1: the instance type doesn't support SGP/trunk interface

Warning  Unsupported              3m20s (x6 over 3m41s)  vpc-resource-controller  The instance type t3.medium is not supported for trunk interface (Security Group for Pods)

case 2: the (mocked) instance is new and hasn't been added into the supporting list

Warning  Unsupported              3m27s (x5 over 3m40s)  vpc-resource-controller  The instance type m5.large is not supported yet by the vpc resource controller

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@haouc haouc requested a review from a team as a code owner May 5, 2023 23:10
Copy link
Contributor

@jdn5126 jdn5126 left a comment

Choose a reason for hiding this comment

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

Changes LGTM, just a few nits and then I can approve

pkg/node/node_test.go Outdated Show resolved Hide resolved
pkg/provider/ip/eni/eni_test.go Outdated Show resolved Hide resolved
pkg/provider/ip/provider.go Outdated Show resolved Hide resolved
pkg/utils/events.go Show resolved Hide resolved
@haouc haouc requested a review from jdn5126 May 8, 2023 16:54
@haouc haouc merged commit 05a2fd3 into aws:master May 8, 2023
@haouc haouc deleted the send-event branch May 8, 2023 18:39
haouc added a commit that referenced this pull request May 29, 2023
* add healthz subpathes for all controllers (#201)

* support arch arg in dockerfile (#207)

* updated vpc limits to include fields for hypervisor type and bare metal status (#217)

* enable node events when instance type is not supported (#218)

* Associate primary network interface SG with the trunk ENI when SG is not specified in ENIConfig (#221)

* Associate primary network interface SG with the trunk ENI when SG is not specified in ENIConfig

* add a new CRD to delegate vpc resource requests (#210)

* upgrade controller runtime version (#227)

* rebased onto master branch

* fixed merge conflict

---------

Co-authored-by: Hao Zhou <[email protected]>
Co-authored-by: Sushmitha Ravikumar <[email protected]>
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.

2 participants