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

Support Cloudstack multiple endpoint for preflight checks #2559

Merged
merged 25 commits into from
Jul 6, 2022
Merged

Support Cloudstack multiple endpoint for preflight checks #2559

Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ require (
github.com/golang/mock v1.6.0
github.com/google/go-github/v35 v35.3.0
github.com/google/uuid v1.3.0
github.com/hashicorp/go-multierror v1.1.1
github.com/mrajashree/etcdadm-controller v1.0.0-rc3
github.com/onsi/gomega v1.19.0
github.com/pkg/errors v0.9.1
Expand Down
20 changes: 16 additions & 4 deletions internal/test/cleanup/cleanup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing it

}
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.

can we defer this close? or does that not work in loops?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think to make defer to work I should pull a function that processes one cmk.

}
return retErr
}

func cleanupCloudStackVms(ctx context.Context, cmk *executables.Cmk, clusterName string, dryRun bool) error {
if err := cmk.ValidateCloudStackConnection(ctx); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
}
20 changes: 15 additions & 5 deletions pkg/dependencies/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ type Dependencies struct {
DockerClient *executables.Docker
Kubectl *executables.Kubectl
Govc *executables.Govc
Cmk *executables.Cmk
Cmks map[string]*executables.Cmk
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cmk is an object combining cmk binary and cloud-config to a certain ACS endpoint. The binary is the same across the list of Cmks but the configurations are different. This way we could keep the most of Cmk.go code as it.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -266,12 +266,17 @@ func (f *Factory) WithProvider(clusterConfigFile string, clusterConfig *v1alpha1
return fmt.Errorf("unable to get machine config from file %s: %v", clusterConfigFile, err)
}

cmkClientMap := cloudstack.CmkClientMap{}
for name, cmk := range f.dependencies.Cmks {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a comment explaining this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Expand Down Expand Up @@ -414,16 +419,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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also check len(f.dependencies.Cmks) > 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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
})
Expand Down
7 changes: 7 additions & 0 deletions pkg/dependencies/factory_test.go
View file
Open in desktop
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 @@ -78,6 +80,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)
deps, err := dependencies.NewFactory().
UseExecutableImage("image:1").
Expand All @@ -96,6 +102,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
View file
Open in desktop
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
View file
Open in desktop
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, config decoder.CloudStackProfileConfig) *Cmk {
return NewCmk(b.buildExecutable(cmkPath), writer, config)
}

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