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

Support Cloudstack multiple endpoint for preflight checks #2559

Merged
merged 25 commits into from
Jul 6, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
bda21a0
Support Cloudstack multiple endpoint for preflight checks
wongni Jun 27, 2022
c464ce9
Update for review comments
wongni Jun 28, 2022
e8e931f
Merge branch 'main' into support-multiple-acs-for-cmk-2
wongni Jun 28, 2022
3efce41
Fix unit test failures
wongni Jun 28, 2022
31679e2
Remove debug print
wongni Jun 28, 2022
461a804
Remove unnecessary file
wongni Jun 28, 2022
dbdac2b
Remove multierror module
wongni Jun 30, 2022
0a87c51
Update to use single cmk object instead of list or map of multiple cmks
wongni Jun 30, 2022
cbd45c9
Remove ValidateCloudStackConnection when cleaning up VMs
wongni Jun 30, 2022
369b53d
Remove NewProviderCustomNet because it's the same as NewProvider
wongni Jun 30, 2022
260ec6e
Avoid using 'failed to' in errors
wongni Jun 30, 2022
5382943
Remove localAvailabilityZones from Validator
wongni Jun 30, 2022
dca6a9e
Remove multipleZone from ValidateNetworkPresent
wongni Jun 30, 2022
074a62c
Return list of errors CleanUpCloudstackTestResources
wongni Jul 1, 2022
bdf278e
Increase unit test coverage
wongni Jul 1, 2022
40c9ddb
Increase unit test coverage #2
wongni Jul 1, 2022
bdbd178
Change error format
wongni Jul 5, 2022
9813e46
Change function names to be more descriptive
wongni Jul 5, 2022
2ddc742
Add default verifySslValue as a constant
wongni Jul 5, 2022
187c194
Fix a bug where empty Zone IDs are used in validateMachineConfig
wongni Jul 5, 2022
0fe30a1
Merge branch 'aws:main' into support-multiple-acs-for-cmk-2
wongni Jul 5, 2022
2ed1d16
Add error message when validating management api endpoint fails
wongni Jul 5, 2022
f6259dd
Make MarkPass message more readable
wongni Jul 5, 2022
3683c61
Merge branch 'aws:main' into support-multiple-acs-for-cmk-2
wongni Jul 6, 2022
222889c
Fix presubmit job failure
wongni Jul 6, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions internal/test/cleanup/cleanup.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,12 @@ func CleanUpCloudstackTestResources(ctx context.Context, clusterName string, dry
if err != nil {
return fmt.Errorf("building cmk executable: %v", err)
}
cmk := executableBuilder.BuildCmkExecutable(tmpWriter, *execConfig)
cmk := executableBuilder.BuildCmkExecutable(tmpWriter, execConfig.Profiles)
defer cmk.Close(ctx)

if err := cmk.ValidateCloudStackConnection(ctx); err != nil {
return fmt.Errorf("validating cloudstack connection with cloudmonkey: %v", err)
for _, profile := range execConfig.Profiles {
if err := cmk.CleanupVms(ctx, profile.Name, clusterName, dryRun); err != nil {
cmk.Close(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we always call cmk.Close() regardless of whether the command itself failed?

Also, I do still feel like we should inform the user if there was an error. Maybe we can concatenate the names of all the profiles we failed to clean up as a string and if the string is not empty, return it as an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defer cmk.Close(ctx) at line 103 will take care of closing of cmk.
I updated the error handling.

}
}

return cmk.CleanupVms(ctx, clusterName, dryRun)
return nil
}
4 changes: 3 additions & 1 deletion pkg/dependencies/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is no longer relevant right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right

f.dependencies.Provider = cloudstack.NewProvider(
datacenterConfig,
machineConfigs,
Expand Down Expand Up @@ -418,12 +419,13 @@ func (f *Factory) WithCmk() *Factory {
if f.dependencies.Cmk != nil {
return nil
}

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.Cmk = f.executableBuilder.BuildCmkExecutable(f.dependencies.Writer, execConfig.Profiles)
f.dependencies.closers = append(f.dependencies.closers, f.dependencies.Cmk)

return nil
Expand Down
7 changes: 7 additions & 0 deletions pkg/dependencies/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package dependencies_test

import (
"context"
"encoding/base64"
"os"
"testing"

Expand All @@ -13,6 +14,7 @@ import (
"github.com/aws/eks-anywhere/pkg/cluster"
"github.com/aws/eks-anywhere/pkg/config"
"github.com/aws/eks-anywhere/pkg/dependencies"
"github.com/aws/eks-anywhere/pkg/providers/cloudstack/decoder"
"github.com/aws/eks-anywhere/pkg/version"
"github.com/aws/eks-anywhere/release/api/v1alpha1"
)
Expand Down Expand Up @@ -107,6 +109,10 @@ func TestFactoryBuildWithClusterManagerWithoutCliConfig(t *testing.T) {
}

func TestFactoryBuildWithMultipleDependencies(t *testing.T) {
configString := test.ReadFile(t, "testdata/cloudstack_config_multiple_profiles.ini")
encodedConfig := base64.StdEncoding.EncodeToString([]byte(configString))
t.Setenv(decoder.EksacloudStackCloudConfigB64SecretKey, encodedConfig)

tt := newTest(t, vsphere)
deps, err := dependencies.NewFactory().
UseExecutableImage("image:1").
Expand All @@ -125,6 +131,7 @@ func TestFactoryBuildWithMultipleDependencies(t *testing.T) {
WithCAPIManager().
WithManifestReader().
WithUnAuthKubeClient().
WithCmk().
Build(context.Background())

tt.Expect(err).To(BeNil())
Expand Down
11 changes: 11 additions & 0 deletions pkg/dependencies/testdata/cloudstack_config_multiple_profiles.ini
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
4 changes: 2 additions & 2 deletions pkg/executables/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ func (b *ExecutableBuilder) BuildGovcExecutable(writer filewriter.FileWriter, op
return NewGovc(b.buildExecutable(govcPath), writer, opts...)
}

func (b *ExecutableBuilder) BuildCmkExecutable(writer filewriter.FileWriter, execConfig decoder.CloudStackExecConfig) *Cmk {
return NewCmk(b.buildExecutable(cmkPath), writer, execConfig)
func (b *ExecutableBuilder) BuildCmkExecutable(writer filewriter.FileWriter, configs []decoder.CloudStackProfileConfig) *Cmk {
return NewCmk(b.buildExecutable(cmkPath), writer, configs)
}

func (b *ExecutableBuilder) BuildAwsCli() *AwsCli {
Expand Down
Loading