Feature: PowerVS cluster creation with dynamic resource creation #1608
Conversation
k8s-ci-robot
added
the
area/provider/ibmcloud
✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
needs-rebase
Karthik-K-N
force-pushed
the
update-ignition
branch
from
dea3da8
to
1fbc757
Compare
k8s-ci-robot
removed
the
needs-rebase
|
team lets review this code by this weekend |
k8s-ci-robot
added
the
needs-rebase
Karthik-K-N
force-pushed
the
update-ignition
branch
from
3bec4e8
to
c2a811a
Compare
k8s-ci-robot
removed
the
needs-rebase
|
Thanks for Dhraneesh for reviewing, I have addressed the comments and marked it as resolved whichever is possible, We will discuss for unresolved comments to finalize the approaches. |
|
|
||
| func (s *PowerVSClusterScope) createCOSBucket() error { | ||
| input := &s3.CreateBucketInput{ | ||
| Bucket: pointer.String(s.COSInstance().BucketName), |
There was a problem hiding this comment.
There's a scenario where when we retry bucket creation immediately with the same name, it throws an error saying bucket exists even though the service instance and bucket get deleted successfully. Maybe we can handle by adding a random string to the bucket name?
(check out util.RandomString() from sigs.k8s.io/cluster-api/util)
Karthik-K-N
force-pushed
the
update-ignition
branch
from
d78c229
to
8fbde63
Compare
go.mod
Outdated
| github.com/IBM/platform-services-go-sdk v0.59.0 | ||
| github.com/IBM/vpc-go-sdk v0.48.0 | ||
| github.com/blang/semver/v4 v4.0.0 | ||
| github.com/coreos/ignition v0.35.0 |
There was a problem hiding this comment.
I see a CVE for this package, need to explore how to fix this? @Prajyot-Parab
There was a problem hiding this comment.
here is the CVE link -- GHSA-hj57-j5cw-2mwp, as this is a very old release, I don't think they will even patch this one. Lets keep this schema offline somewhere in the pkg folder.
There was a problem hiding this comment.
fix - Karthik-K-N#12
| if s.ResourceGroup() == nil || s.ResourceGroup().Name == nil { | ||
| return "", fmt.Errorf("resource group name is not set") | ||
| } |
There was a problem hiding this comment.
TODO: Use default resource group for the account if not specified.
| // GetBootstrapData returns the base64 encoded bootstrap data from the secret in the Machine's bootstrap.dataSecretName. | ||
| func (m *PowerVSMachineScope) GetBootstrapData() (string, error) { | ||
| // DeleteMachineIgnition deletes the ignition associated with machine. | ||
| func (m *PowerVSMachineScope) DeleteMachineIgnition() error { |
There was a problem hiding this comment.
@Karthik-K-N did you drop it by mistake? I have it in my other PR, if need be I can split it in a diff PR and push.
There was a problem hiding this comment.
Intentioally removed it for this PR, Lets add it along with ignition 2.4 support PR. For now cosInstance delete will automatically delete all the buckets within it.
There was a problem hiding this comment.
Updated it in the latest change to delete ignition as part of machine deletion.
Karthik-K-N
force-pushed
the
update-ignition
branch
2 times, most recently
from
df44c4a
to
274f8f9
Compare
|
@Prajyot-Parab @Karthik-K-N I see linting issue for that file, see if we can somehow ignore that for that entire package |
fixed. |
Karthik-K-N
force-pushed
the
update-ignition
branch
from
6b7599e
to
b847c57
Compare
Signed-off-by: Prajyot-Parab <prajyot.parab2@ibm.com>
Karthik-K-N
force-pushed
the
update-ignition
branch
from
b847c57
to
825789b
Compare
k8s-ci-robot
added
the
approved
|
I have approved the PR, one of you @Amulyam24 @Prajyot-Parab give lgtm |
|
@Karthik-K-N: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Karthik-K-N, mkumatag, Prajyot-Parab 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 |
What this PR does / why we need it:
This PR adds the ability to create required resources for creating Power VS cluster dynamically eliminating user to precreate resources before creating cluster. A reference to proposal has been added which contains the detailed implementation plan for this feature.
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 ##1611
Special notes for your reviewer:
/area provider/ibmcloud
Release note:
Proposal: #1488
API Pr: #1485
#1592
TODO: