-
Notifications
You must be signed in to change notification settings - Fork 287
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
Support Cloudstack multiple endpoint for preflight checks #2559
Changes from 6 commits
bda21a0
c464ce9
e8e931f
3efce41
31679e2
461a804
dbdac2b
0a87c51
cbd45c9
369b53d
260ec6e
5382943
dca6a9e
074a62c
bdf278e
40c9ddb
bdbd178
9813e46
2ddc742
187c194
0fe30a1
2ed1d16
f6259dd
3683c61
222889c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ import ( | |
"strconv" | ||
|
||
"github.com/aws/aws-sdk-go/aws/session" | ||
"github.com/hashicorp/go-multierror" | ||
|
||
"github.com/aws/eks-anywhere/internal/pkg/ec2" | ||
"github.com/aws/eks-anywhere/internal/pkg/s3" | ||
|
@@ -85,7 +86,7 @@ func VsphereRmVms(ctx context.Context, clusterName string, opts ...executables.G | |
return govc.CleanupVms(ctx, clusterName, false) | ||
} | ||
|
||
func CleanUpCloudstackTestResources(ctx context.Context, clusterName string, dryRun bool) error { | ||
func CleanUpCloudstackTestResources(ctx context.Context, clusterName string, dryRun bool) (retErr error) { | ||
executableBuilder, close, err := executables.NewExecutableBuilder(ctx, executables.DefaultEksaImage()) | ||
if err != nil { | ||
return fmt.Errorf("unable to initialize executables: %v", err) | ||
|
@@ -99,12 +100,23 @@ func CleanUpCloudstackTestResources(ctx context.Context, clusterName string, dry | |
if err != nil { | ||
return fmt.Errorf("building cmk executable: %v", err) | ||
} | ||
cmk := executableBuilder.BuildCmkExecutable(tmpWriter, *execConfig) | ||
defer cmk.Close(ctx) | ||
for _, config := range execConfig.Profiles { | ||
cmk := executableBuilder.BuildCmkExecutable(tmpWriter, config) | ||
if err := cleanupCloudStackVms(ctx, cmk, clusterName, dryRun); err != nil { | ||
retErr = multierror.Append(retErr, err) | ||
} | ||
cmk.Close(ctx) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we defer this close? or does that not work in loops? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think to make |
||
} | ||
return retErr | ||
} | ||
|
||
func cleanupCloudStackVms(ctx context.Context, cmk *executables.Cmk, clusterName string, dryRun bool) error { | ||
if err := cmk.ValidateCloudStackConnection(ctx); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need to validate cloudstack connection? the cleanupVms will fail w same connection error anyway if there's connection issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing ValidateCloudStackConnection |
||
return fmt.Errorf("validating cloudstack connection with cloudmonkey: %v", err) | ||
} | ||
|
||
return cmk.CleanupVms(ctx, clusterName, dryRun) | ||
if err := cmk.CleanupVms(ctx, clusterName, dryRun); err != nil { | ||
return fmt.Errorf("cleaning up VMs with cloudmonkey: %v", err) | ||
} | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,7 +50,7 @@ type Dependencies struct { | |
DockerClient *executables.Docker | ||
Kubectl *executables.Kubectl | ||
Govc *executables.Govc | ||
Cmk *executables.Cmk | ||
Cmks map[string]*executables.Cmk | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to build a specific binary for each of the configs? Can't this just be solved by setting the appropriate envvar per command execution somehow? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cmk is an object combining There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can cmk exec takes profile for each cmd? So that we do not need to preset the configuration and have a map of exact same exec. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to use single cmk instead of multiple cmk objects |
||
SnowAwsClient aws.Clients | ||
SnowConfigManager *snow.ConfigManager | ||
Writer filewriter.FileWriter | ||
|
@@ -267,12 +267,18 @@ func (f *Factory) WithProvider(clusterConfigFile string, clusterConfig *v1alpha1 | |
return fmt.Errorf("unable to get machine config from file %s: %v", clusterConfigFile, err) | ||
} | ||
|
||
// map[string]*executables.Cmk and map[string]ProviderCmkClient are not compatible so we convert the map manually | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this comment is no longer relevant right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right |
||
cmkClientMap := cloudstack.CmkClientMap{} | ||
for name, cmk := range f.dependencies.Cmks { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you add a comment explaining this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
cmkClientMap[name] = cmk | ||
} | ||
|
||
f.dependencies.Provider = cloudstack.NewProvider( | ||
datacenterConfig, | ||
machineConfigs, | ||
clusterConfig, | ||
f.dependencies.Kubectl, | ||
f.dependencies.Cmk, | ||
cmkClientMap, | ||
f.dependencies.Writer, | ||
time.Now, | ||
skipIpCheck, | ||
|
@@ -415,16 +421,21 @@ func (f *Factory) WithCmk() *Factory { | |
f.WithExecutableBuilder().WithWriter() | ||
|
||
f.buildSteps = append(f.buildSteps, func(ctx context.Context) error { | ||
if f.dependencies.Cmk != nil { | ||
if f.dependencies.Cmks != nil && len(f.dependencies.Cmks) > 0 { | ||
return nil | ||
} | ||
f.dependencies.Cmks = map[string]*executables.Cmk{} | ||
|
||
execConfig, err := decoder.ParseCloudStackSecret() | ||
if err != nil { | ||
return fmt.Errorf("building cmk executable: %v", err) | ||
} | ||
|
||
f.dependencies.Cmk = f.executableBuilder.BuildCmkExecutable(f.dependencies.Writer, *execConfig) | ||
f.dependencies.closers = append(f.dependencies.closers, f.dependencies.Cmk) | ||
for _, profileConfig := range execConfig.Profiles { | ||
cmk := f.executableBuilder.BuildCmkExecutable(f.dependencies.Writer, profileConfig) | ||
f.dependencies.Cmks[profileConfig.Name] = cmk | ||
f.dependencies.closers = append(f.dependencies.closers, cmk) | ||
} | ||
|
||
return nil | ||
}) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
[Global] | ||
verify-ssl = false | ||
api-key = test-key1 | ||
secret-key = test-secret1 | ||
api-url = http://127.16.0.1:8080/client/api | ||
|
||
[Instance2] | ||
verify-ssl = true | ||
api-key = test-key2 | ||
secret-key = test-secret2 | ||
api-url = http://127.16.0.2:8080/client/api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this error look? Just wondering if we need to import a new library or solve it a different way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The multierror module is useful when we want to continue to clean up the resources on encountering an error so that we can clean up as many resources as possible. The appended errors are integrated into a single error. If you think introducing a new module just for testing isn't a good idea I can remove multierror module from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my vote would be removing this third party module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing it