-
Notifications
You must be signed in to change notification settings - Fork 69
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
Multi cloud e2e master rebase #568
Multi cloud e2e master rebase #568
Conversation
Skipping CI for Draft Pull Request. |
/test all |
7a39fd2
to
38ec7a9
Compare
Codecov Report
@@ Coverage Diff @@
## master #568 +/- ##
==========================================
- Coverage 38.43% 37.72% -0.72%
==========================================
Files 13 13
Lines 2849 2908 +59
==========================================
+ Hits 1095 1097 +2
- Misses 1673 1728 +55
- Partials 81 83 +2
Continue to review full report at Codecov.
|
OADP_CRED_FILE = ${OADP_CRED_DIR}/gcp-credentials | ||
CREDS_SECRET_REF = cloud-credentials-gcp | ||
OADP_BUCKET_FILE = ${OADP_CRED_DIR}/gcp-velero-bucket-name | ||
else ifeq ($(CLUSTER_TYPE), azure4) |
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.
azure4, is that the real cluster type or typo?
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.
Yes, thats the real cluster type @weshayutin. Changing it on the next line to a proper one.
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.
This looks good, one stupid question in my comments... re: azure4.
Thank you Deepak!
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.
VISIACK, looks good
/test 4.7-operator-e2e |
config/manager/kustomization.yaml
Outdated
@@ -12,5 +12,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1 | |||
kind: Kustomization | |||
images: | |||
- name: controller | |||
newName: quay.io/konveyor/oadp-operator | |||
newTag: latest | |||
newName: quay.io/deepakraj1997/oadp-operator |
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.
do we want to change that?
@@ -563,6 +378,9 @@ var _ = Describe("Configuration testing for DPA Custom Resource", func() { | |||
}, | |||
WantError: false, | |||
}, nil), | |||
} | |||
|
|||
awsTests := []TableEntry{ |
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.
is it possible to use labels instead? for Example, add in the Makefile:
FILTER_PROVIDER :=
And then mark:
Entry("AWS Without Region No S3ForcePathStyle with BackupImages false should succeed", Label("aws-provider"), InstallCase{
@@ -661,5 +480,132 @@ var _ = Describe("Configuration testing for DPA Custom Resource", func() { | |||
}, | |||
WantError: true, | |||
}, fmt.Errorf("region for AWS backupstoragelocation cannot be empty when s3ForcePathStyle is true or when backing up images")), | |||
} | |||
|
|||
if provider == "aws" { |
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.
same as the comment above
Expect(err).NotTo(HaveOccurred()) | ||
if len(installCase.DpaSpec.BackupLocations) > 0 { | ||
switch dpaCR.Provider { | ||
case "aws": |
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.
You can feed this data to the json instead, and then you won't have to switch over the provider type; this way it will be initialized generically inside dpaCr.build function:
Velero: &velero.BackupStorageLocationSpec{
Provider: v.CustomResource.Spec.BackupLocations[0].Velero.Provider,
Default: true,
Config: v.CustomResource.Spec.BackupLocations[0].Velero.Config,
Credential: v.CustomResource.Spec.BackupLocations[0].Velero.Credential,
) | ||
|
||
// Common vars obtained from flags passed in ginkgo. | ||
var namespace, instanceName, settings, cloud, clusterProfile, credSecretRef string | ||
var credFile, namespace, credSecretRef, instanceName, provider, azure_resource_file, openshift_ci, ci_cred_file, settings, bsl_profile string |
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.
is it possible to feed all platform specific vars to the json file instead? it's better to keep the tests platform agnostic as possible
-velero_instance_name=$(VELERO_INSTANCE_NAME) \ | ||
-timeout_multiplier=$(E2E_TIMEOUT_MULTIPLIER) \ | ||
-cluster_profile=$(CLUSTER_PROFILE) \ | ||
--ginkgo.label-filter="$(TEST_FILTER)" | ||
-cluster_profile=$(CLUSTER_TYPE) \ |
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.
is it possible to feed all platform specific info to the json file instead? better to keep it platform agnostic...
// }, | ||
// } | ||
case "azure": | ||
dpaInstance.Spec.BackupLocations[0].Velero.Config = map[string]string{ |
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.
is it possible to feed this info to the json instead? this way the code above already covers this initialization:
BackupLocations: []oadpv1alpha1.BackupLocation{
{
Velero: &velero.BackupStorageLocationSpec{
Provider: v.CustomResource.Spec.BackupLocations[0].Velero.Provider,
Default: true,
Config: v.CustomResource.Spec.BackupLocations[0].Velero.Config, <<<<<<<<<<
Credential: v.CustomResource.Spec.BackupLocations[0].Velero.Credential,
Key: "cloud", | ||
} | ||
} | ||
dpaInstance.Spec.SnapshotLocations = []oadpv1alpha1.SnapshotLocation{ |
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.
is it possible to feed this info to the json instead? this way the code above already covers this initialization:
SnapshotLocations: v.CustomResource.Spec.SnapshotLocations, <<<<<<<<<<<<<<<<<<<<<
BackupLocations: []oadpv1alpha1.BackupLocation{
{
Velero: &velero.BackupStorageLocationSpec{
Provider: v.CustomResource.Spec.BackupLocations[0].Velero.Provider,
Default: true,
Config: v.CustomResource.Spec.BackupLocations[0].Velero.Config,
Credential: v.CustomResource.Spec.BackupLocations[0].Velero.Credential,
@@ -84,6 +98,73 @@ func (v *DpaCustomResource) Build(backupRestoreType BackupRestoreType) error { | |||
}, | |||
}, | |||
} | |||
switch v.Provider { | |||
case "aws": |
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.
can do instead of this switch:
if v.OpenshiftCi {
if dpaInstance.Spec.BackupLocations[0].Velero.Config != nil {
dpaInstance.Spec.BackupLocations[0].Velero.Config["credentialsFile"] = fmt.Sprintf(""bsl-cloud-credentials-%v/cloud"+line+"\n", v.Provider)
}
} else {
dpaInstance.Spec.BackupLocations[0].Velero.Credential = &corev1.SecretKeySelector{
LocalObjectReference: corev1.LocalObjectReference{
Name: v.CredSecretRef,
},
Key: "cloud",
}
}
{ | ||
"velero": { | ||
"provider": "$PROVIDER", | ||
"config": { |
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.
is it possible to feed the info from the makefile to here and use it throughout the tests instead of passing it as arguments? this way the code would be more generic
|
||
Expect(err).NotTo(HaveOccurred()) | ||
dpaCR.DpaAzureConfig = DpaAzureConfig{ |
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.
is it possible to feed this info to the json script?
if openshift_ci_bool == true { | ||
switch dpaCR.Provider { | ||
case "aws": | ||
cloudCredData, err := utils.ReadFile(dpaCR.Credentials) |
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.
Instead of switching over each provider, can do:
cloudCredData, err := utils.ReadFile(dpaCR.Credentials)
Expect(err).NotTo(HaveOccurred())
err = CreateCredentialsSecret(cloudCredData, namespace, fmt.Sprintf("bsl-cloud-credentials-$v", dpaCr.Provider)
Expect(err).NotTo(HaveOccurred())
dpaCR.Credentials = ci_cred_file
tests/e2e/lib/kube_helpers.go
Outdated
return resourceGroup, err | ||
} | ||
|
||
func WriteFile(credFile string, data []byte) error { |
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.
belongs more to common.go
tests/e2e/lib/kube_helpers.go
Outdated
@@ -217,3 +219,46 @@ func isCredentialsSecretDeleted(namespace string, credSecretRef string) wait.Con | |||
return false, err | |||
} | |||
} | |||
|
|||
func GetJsonData(path string) (map[string]interface{}, error) { |
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.
belongs more to common.go
@@ -37,13 +37,27 @@ const ( | |||
RESTIC BackupRestoreType = "restic" | |||
) | |||
|
|||
type DpaAzureConfig struct { |
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.
can be read from the json instead to keep the code platform agnostic
type DpaCustomResource struct { | ||
Name string | ||
Namespace string | ||
SecretName string | ||
backupRestoreType BackupRestoreType | ||
CustomResource *oadpv1alpha1.DataProtectionApplication | ||
Client client.Client | ||
DpaAzureConfig DpaAzureConfig |
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.
can be read from the json instead to keep the code platform agnostic
@deepakraj1997: all tests passed! Full PR test history. Your PR dashboard. 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. |
Let's land this and start working through @mperetzred's comments. 3 e2e passes in a row +2 approvals. Commiting the change will free up others e2e patches that are pending. |
@weshayutin sure. |
Thanks @mperetzred . To summarize my response to your review comments, The reason I have I will work on this incrementally. Merging this @weshayutin |
# bsl cred file | ||
OADP_CRED_FILE ?= ${OADP_CRED_DIR}/new-aws-credentials | ||
# vsl cred file | ||
CI_CRED_FILE ?= ${CLUSTER_PROFILE_DIR}/.awscred |
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.
After much confusion debugging ci.. this probably should've been called VSL_CRED_FILE
* Adding changes from ci-multi-cloud branch * Updating flags in makefile * Updating flags in makefile * Updating flags in makefile * Update defaults * Fix helpers * Cleaning up makefile * Update makefile * Update kustomization * Cleaning up Makefile * Adding changes for AWS CredentialFile * Handling no default backuplocations * Removing comments * Adding config check in helpers * Fix AWS Test case * Fix basic review comments
* Adding changes from ci-multi-cloud branch * Updating flags in makefile * Updating flags in makefile * Updating flags in makefile * Update defaults * Fix helpers * Cleaning up makefile * Update makefile * Update kustomization * Cleaning up Makefile * Adding changes for AWS CredentialFile * Handling no default backuplocations * Removing comments * Adding config check in helpers * Fix AWS Test case * Fix basic review comments
…n updateRegistrySecret (#679) * Replace carriage return when getting provider secrets for registry + E2E changes (#652) * Replace carriage return when getting provider secrets for registry Co-authored-by: GitHub Co-pilot <idkwhatemailgithubcopilotuses@github.com> * add tests * replaceCarriageReturn unit test * Registry credentials can use other secret names Co-authored-by: GitHub Co-pilot <idkwhatemailgithubcopilotuses@github.com> * Multi cloud E2E Updates (#568) * Adding changes from ci-multi-cloud branch * Updating flags in makefile * Updating flags in makefile * Updating flags in makefile * Update defaults * Fix helpers * Cleaning up makefile * Update makefile * Update kustomization * Cleaning up Makefile * Adding changes for AWS CredentialFile * Handling no default backuplocations * Removing comments * Adding config check in helpers * Fix AWS Test case * Fix basic review comments * genericTests... * Changing Credentials Mount Name for BSL when CredentialsFile key is used in Config - Fixes CI (#637) * Changing Credentials Mount Name for BSL for GCP * Fix DefaultPlugin for credentialsFile * E2E Tests: Fix azure templating name. (#573) * Fixing Azure to run locally * Aligning json * Handling Azure Parameters in CI Env using templates. (#582) * Adding azure parameters * Adding Openshift CI params * Update Test Suite and Helpers * Adding cred ref * Adding cred ref * Changing azure oadp cred dir to tmp * Fixing duplicate error * Removing OpenShift CI * Changing test instance name * Changing the AWS BSL Profile to default * Changing BslMountPath variable in registry controller * Replace carriage return when getting provider secrets for registry + E2E changes (#652) * Replace carriage return when getting provider secrets for registry Co-authored-by: GitHub Co-pilot <idkwhatemailgithubcopilotuses@github.com> * add tests * replaceCarriageReturn unit test * Registry credentials can use other secret names Co-authored-by: GitHub Co-pilot <idkwhatemailgithubcopilotuses@github.com> * fix missing err return for updateRegistrySecret function * go fmt * upgraded ginkgo on tests to ginkgo 2.0 (#560) Co-authored-by: Nitish Srivastava <43022982+nitishSr@users.noreply.github.com> Co-authored-by: GitHub Co-pilot <idkwhatemailgithubcopilotuses@github.com> Co-authored-by: Deepak Raj D S <drajds@redhat.com> Co-authored-by: Shubham Pampattiwar <shubhampampattiwar7@gmail.com> Co-authored-by: Maya Peretz <mperetz@redhat.com>
https://hackmd.io/Gzmvnu90RC20f-6OYnwskA