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

Check Fargate Profile existence before creating #5722

Merged
merged 4 commits into from Jan 24, 2023
Merged

Check Fargate Profile existence before creating #5722

merged 4 commits into from Jan 24, 2023

Conversation

guessi
Copy link
Contributor

@guessi guessi commented Sep 24, 2022 •
edited

Description

When creating Fargate Profile from ClusterConfig file, it might need to comment out existed on then create a new one. With this PR, an existed Fargate Profile creation will be skipped before start.

Generally, we will create a cluster with Fargate Profile with ClusterConfig,

apiVersion: eksctl.io/v1alpha5
kind: ClusterConfig

metadata:
  name: eks-demo
  region: us-east-1

fargateProfiles:
  - name: demo-existed-profile
    selectors:
      - namespace: itdoesntmatter1

However, if we try to create an new one with the following ClusterConfig,

apiVersion: eksctl.io/v1alpha5
kind: ClusterConfig

metadata:
  name: eks-demo
  region: us-east-1

fargateProfiles:
  - name: demo-existed-profile # <--------- existed one still there.
    selectors:
      - namespace: itdoesntmatter1

  - name: demo-new-profile # <--------- new Fargate Profile.
    selectors:
      - namespace: itdoesntmatter2

It would get 409 conflict error prompt as follow,

2022-XX-XX ... Error: could not create fargate profiles: ..., https response error StatusCode: 409, RequestID: ..., api error ResourceInUseException: A Fargate Profile already exists with this name in this cluster.

With this PR changes, it should skip existed Fargate Profiles but not
giving 409 Conflict error return.

% make binary

% ./eksctl version
0.122.0-dev+a5d7c20ba.2022-11-30T20:36:42Z

% ./eksctl create fargateprofile -f ./cluster-config/cluster-minimal.yaml
2022-11-30 20:50:12 [ℹ]  Fargate profile "demo-existed-profile" already exists on EKS cluster "eks-demo"
2022-11-30 20:50:12 [ℹ]  creating Fargate profile "demo-new-profile" on EKS cluster "eks-demo"

...

Fixes: #5652

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes
  • (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@guessi guessi marked this pull request as draft September 25, 2022 02:33
@guessi guessi changed the title Check Fargate Profile existence while creating WIP: Check Fargate Profile existence while creating Sep 25, 2022
@guessi
Copy link
Contributor Author

guessi commented Sep 25, 2022 •
edited

Failed for the unit-test, set as "WIP" temporary and I will try to wok on it later.

@guessi guessi marked this pull request as ready for review September 25, 2022 06:43
@guessi guessi changed the title WIP: Check Fargate Profile existence while creating Check Fargate Profile existence while creating Sep 25, 2022
Himangini
Himangini reviewed Sep 26, 2022
View reviewed changes
Copy link
Collaborator

@Himangini Himangini left a comment

Choose a reason for hiding this comment

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

Can you add the link of the issue in the PR description, also please add the config file used for your manual testing

pkg/eks/fargate.go Outdated Show resolved Hide resolved
pkg/eks/fargate.go Outdated Show resolved Hide resolved
@guessi guessi force-pushed the check-fargate-existence branch 2 times, most recently from d9ea9ae to 4ec539f Compare October 12, 2022 14:16
@guessi
Copy link
Contributor Author

guessi commented Nov 27, 2022

once again, rebased upstream/release-0.121

Himangini
Himangini requested changes Nov 29, 2022
View reviewed changes
Copy link
Collaborator

@Himangini Himangini left a comment

Choose a reason for hiding this comment

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

Left some suggestions, also please add tests for your changes! 👍🏻

docs/release_notes/0.121.0.md Outdated Show resolved Hide resolved
pkg/eks/fargate.go Outdated Show resolved Hide resolved
pkg/eks/fargate.go Outdated Show resolved Hide resolved
pkg/eks/fargate.go Outdated Show resolved Hide resolved
@guessi guessi force-pushed the check-fargate-existence branch 3 times, most recently from 5048c79 to cd1e8a9 Compare November 30, 2022 13:41
guessi
guessi commented Nov 30, 2022
View reviewed changes
pkg/eks/fargate_test.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the stale label Dec 31, 2022
@Himangini Himangini removed the stale label Jan 4, 2023
Himangini
Himangini reviewed Jan 9, 2023
View reviewed changes
Copy link
Collaborator

@Himangini Himangini left a comment

Choose a reason for hiding this comment

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

thanks for adding all the changes and apologies for the late review, i was away on holiday and didn't get a chance to come back to you. I have added some comments but I think your PR is nearly there. small changes and it almost looks good to merge. ✨

pkg/eks/fargate.go Outdated Show resolved Hide resolved
pkg/eks/fargate_test.go Outdated Show resolved Hide resolved
pkg/eks/fargate.go Show resolved Hide resolved
pkg/eks/fargate_test.go Outdated Show resolved Hide resolved
@guessi
Check Fargate Profile existence while creating
56c7e95 
When creating Fargate Profile from ClusterConfig file, it might need to
comment out existed on then create a new one. With this PR, an existed
Fargate Profile creation will be skipped before start.

```
% make binary
```

```
% ./eksctl version
0.122.0-dev+a5d7c20ba.2022-11-30T20:36:42Z
```

```
% ./eksctl create fargateprofile -f ./cluster-config/cluster-minimal.yaml
2022-11-30 20:50:12 [ℹ]  Fargate profile "demo-existed-profile" already exists on EKS cluster "eks-demo"
2022-11-30 20:50:12 [ℹ]  creating Fargate profile "demo-new-profile" on EKS cluster "eks-demo"

...
```
@guessi
Remove unnecessary tests
6c1a1a1
@guessi
Copy link
Contributor Author

guessi commented Jan 14, 2023

@Himangini Just update PR with rebase release-0.125. Please review the changes while you are free, thanks!

@Himangini
Copy link
Collaborator

@Himangini Just update PR with rebase release-0.125. Please review the changes while you are free, thanks!

thanks I am going to test the PR, and once I am happy will get this merged.

@Himangini Himangini changed the title Check Fargate Profile existence while creating Check Fargate Profile existence before creating Jan 24, 2023
Himangini
Himangini approved these changes Jan 24, 2023
View reviewed changes
Copy link
Collaborator

@Himangini Himangini left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@Himangini Himangini merged commit f0cfcc8 into eksctl-io:main Jan 24, 2023
@guessi guessi deleted the check-fargate-existence branch January 25, 2023 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Himangini Himangini Himangini approved these changes

Assignees
No one assigned
Labels
area/fargate kind/bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Error: could not create fargate profiles: failed to create Fargate profile "fp-default" on EKS cluster
2 participants
@guessi @Himangini