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

add observability topology for PowerScale, UT, e2e and etc #105

Merged
merged 1 commit into from
Oct 13, 2022

Conversation

YianZong
Copy link
Contributor

Description

add observability topology for PowerScale, UT, e2e and etc

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#488

Checklist:

  • I have performed a self-review of my own code to ensure there are no formatting, vetting, linting, or security issues
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate
  • I have maintained at least 90% code coverage
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have maintained backward compatibility

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

  • UT
    Current coverage:
    [root@vpi6179 controllers]# go test ./... -coverprofile cover.out ok github.com/dell/csm-operator/controllers 0.591s coverage: 88.5% of statements [root@vpi6179 modules]# go test ./... -coverprofile cover.out ok github.com/dell/csm-operator/pkg/modules 0.105s coverage: 88.9% of statements
  • E2E
    `Running Suite: CSM Operator End-to-End Tests
    ============================================
    Random Seed: 1664430799 - Will randomize all specs
    Will run 1 of 1 specs

STEP: Getting test environment variables
STEP: Reading values file
STEP: Getting a k8s client
[run-e2e-test]E2E Testing
Running all test Given Test Scenarios
/home/dellemc/csm-operator/tests/e2e/e2e_test.go:75
[It] Running all test Given Test Scenarios
/home/dellemc/csm-operator/tests/e2e/e2e_test.go:75
STEP: Starting: Install PowerScale Driver(With Observability)
STEP: Executing Given an environment with k8s or openshift, and CSM operator installed
STEP: Executing Apply custom resources
Sep 29 01:53:23.261: INFO: Running '/usr/bin/kubectl --namespace=test-isilon apply --validate=true -f -'
Sep 29 01:53:23.507: INFO: stderr: ""
Sep 29 01:53:23.507: INFO: stdout: "containerstoragemodule.storage.dell.com/test-isilon created\n"
STEP: Executing Validate custom resources
STEP: Executing Validate [powerscale] driver is installed
STEP: Executing Validate [observability] module is installed
STEP: Executing Enable forceRemoveDriver on CR
STEP: Executing Delete resources
STEP: Ending: Install PowerScale Driver(With Observability)
STEP:
STEP:
STEP: Starting: Install PowerScale Driver(Standalone), Enable Observability
STEP: Executing Given an environment with k8s or openshift, and CSM operator installed
STEP: Executing Apply custom resources
Sep 29 01:53:38.553: INFO: Running '/usr/bin/kubectl --namespace=test-isilon apply --validate=true -f -'
Sep 29 01:53:38.802: INFO: stderr: ""
Sep 29 01:53:38.802: INFO: stdout: "containerstoragemodule.storage.dell.com/test-isilon created\n"
STEP: Executing Validate custom resources
STEP: Executing Validate [powerscale] driver is installed
STEP: Executing Validate [observability] module is not installed
STEP: Executing Enable [observability] module
STEP: Executing Validate [powerscale] driver is installed
STEP: Executing Validate [observability] module is installed
STEP: Executing Enable forceRemoveDriver on CR
STEP: Executing Delete resources
STEP: Ending: Install PowerScale Driver(Standalone), Enable Observability
STEP:
STEP:
STEP: Starting: Install PowerScale Driver(With Observability), Disable Observability module"
STEP: Executing Given an environment with k8s or openshift, and CSM operator installed
STEP: Executing Apply custom resources
Sep 29 01:53:53.878: INFO: Running '/usr/bin/kubectl --namespace=test-isilon apply --validate=true -f -'
Sep 29 01:53:54.166: INFO: stderr: ""
Sep 29 01:53:54.166: INFO: stdout: "containerstoragemodule.storage.dell.com/test-isilon created\n"
STEP: Executing Validate custom resources
STEP: Executing Validate [powerscale] driver is installed
STEP: Executing Validate [observability] module is installed
STEP: Executing Disable [observability] module
STEP: Executing Validate [powerscale] driver is installed
STEP: Executing Validate [observability] module is not installed
STEP: Executing Enable forceRemoveDriver on CR
STEP: Executing Delete resources
STEP: Ending: Install PowerScale Driver(With Observability), Disable Observability module"
STEP:
STEP:

• [SLOW TEST:55.986 seconds]
[run-e2e-test]E2E Testing
/home/dellemc/csm-operator/tests/e2e/e2e_test.go:74
Running all test Given Test Scenarios
/home/dellemc/csm-operator/tests/e2e/e2e_test.go:75

Ran 1 of 1 Specs in 56.344 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS

Ginkgo ran 1 suite in 59.587013979s
Test Suite Passed

`

  • Manual
    image

@rajendraindukuri
Copy link
Collaborator

Hi @YianZong Please do not merge this PR as posting activities are in progress for CSM Operator. Once those are done, we can merge. Blocking this PR for now.

@@ -10,3 +10,4 @@ powerscale:
v2.4.0:
authorization: "v1.4.0"
replication: "v1.3.0"
observability: "v1.3.0"

Choose a reason for hiding this comment

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

different obs modules have different versions, shall we use more fine-graded key name? e.g. obs-topology?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@forrestxia this is observability module version 1.3.0, rather than individual component image version

Choose a reason for hiding this comment

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

observability module version is an abstract concept-only version. If so, shall we use v1.4.0 for the coming release? @YianZong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@forrestxia We add v1.3.0 first, then add v1.4.0 for Q4 release.

Choose a reason for hiding this comment

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

ok, see. thanks for explanation.

@YianZong
Copy link
Contributor Author

Hi @YianZong Please do not merge this PR as posting activities are in progress for CSM Operator. Once those are done, we can merge. Blocking this PR for now.

Hi @rajendraindukuri, when will the posting be done?

@rajendraindukuri
Copy link
Collaborator

Hi @YianZong Please do not merge this PR as posting activities are in progress for CSM Operator. Once those are done, we can merge. Blocking this PR for now.

Hi @rajendraindukuri, when will the posting be done?

Hi @YianZong Since RTS was done yesterday. We started posting today.. If all goes good, should be done within a week. Will keep you posted

samples/storage_csm_powerscale_v240.yaml Show resolved Hide resolved
pkg/modules/observability.go Outdated Show resolved Hide resolved
@YianZong YianZong force-pushed the feature-488-observability-topology branch from ba1e8a8 to b66e131 Compare October 9, 2022 07:01
@YianZong
Copy link
Contributor Author

Hi Code Owners, please kindly review the PR and let me know if any change needed. Thank you!

@forrestxia
Copy link

@YianZong "Check for forbidden words" seems required, and no result yet, please check what to be done.

@YianZong
Copy link
Contributor Author

@YianZong "Check for forbidden words" seems required, and no result yet, please check what to be done.

@forrestxia The check of "Check for forbidden words" is commented as removed in the actions.yml.

@YianZong YianZong force-pushed the feature-488-observability-topology branch from b66e131 to ea01f1b Compare October 11, 2022 13:22
@YianZong
Copy link
Contributor Author

Rebased main branch

@YianZong YianZong force-pushed the feature-488-observability-topology branch from 478fca7 to f545be8 Compare October 12, 2022 09:42
@YianZong
Copy link
Contributor Author

Rebased main branch again.
Hi Code Owners,
Please help review and approve this PR. Other PRs of Observability are all based on this code change.
Thank you!

taohe1012
taohe1012 previously approved these changes Oct 12, 2022
baoy1
baoy1 previously approved these changes Oct 12, 2022
rensyct
rensyct previously approved these changes Oct 12, 2022
@@ -30,6 +30,9 @@ type DriverType string
// ModuleType - type representing the type of the modules. e.g. - authorization, podmon
type ModuleType string

// OberservabilityComponentType - type representing the type of components inside observability module. e.g. - topology
type OberservabilityComponentType string
Copy link
Contributor

Choose a reason for hiding this comment

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

spell check Oberservability -> Observability

@@ -30,6 +30,9 @@ type DriverType string
// ModuleType - type representing the type of the modules. e.g. - authorization, podmon
type ModuleType string

// OberservabilityComponentType - type representing the type of components inside observability module. e.g. - topology
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as below

@@ -49,6 +52,9 @@ const (
// ReverseProxy - placeholder for constant csireverseproxy
ReverseProxy ModuleType = "csireverseproxy"

// Topology - placeholder for constant topology
Topology OberservabilityComponentType = "topology"

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above

}
for _, cluster := range clusterClients {
// remove module observability
log.Infow("Deleteing observability")
Copy link
Contributor

Choose a reason for hiding this comment

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

spell check Deleteing -> deleting

@@ -780,6 +821,14 @@ func (r *ContainerStorageModuleReconciler) removeDriver(ctx context.Context, ins
}
}

// remove module observability
if observabilityEnabled, _ := utils.IsModuleEnabled(ctx, instance, r, csmv1.Observability); observabilityEnabled {
log.Infow("Deleteing observability")
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above

spec:
driver:
csiDriverType: "isilon"
configVersion: v2.2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not be 2.4.0?

authSecret: test-isilon-creds
replicas: 1
common:
image: "dellemc/csi-isilon:v2.2.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above

@bharathsreekanth
Copy link
Collaborator

@YianZong v1alpha1 to v1 changes will affect some of the code in here. I personally preferred to have that change go in first, so that all the teams can do the changes needed based on that. Let us discuss this in the meeting today to see how to coordinate.

@YianZong
Copy link
Contributor Author

@YianZong v1alpha1 to v1 changes will affect some of the code in here. I personally preferred to have that change go in first, so that all the teams can do the changes needed based on that. Let us discuss this in the meeting today to see how to coordinate.

Hi @bharathsreekanth My understanding is that 'v1alpha1 to v1' change is the CSM API/CRD version update, it's not conflict with other PRs' code changes. But, I'm ok to wait if that PR can be merged as soon as possible.

@YianZong YianZong dismissed stale reviews from rensyct, baoy1, and taohe1012 via 84db5af October 12, 2022 13:06
@YianZong YianZong force-pushed the feature-488-observability-topology branch from f545be8 to 84db5af Compare October 12, 2022 13:06
@YianZong
Copy link
Contributor Author

Thanks, @rensyct all addressed.

@YianZong YianZong force-pushed the feature-488-observability-topology branch from 84db5af to 9688834 Compare October 13, 2022 05:15
@YianZong
Copy link
Contributor Author

Rebased main branch and updated v1alpha to v1 in newly added files.
Manually verified again:
image

@rensyct rensyct merged commit ea0d0a6 into main Oct 13, 2022
@YianZong YianZong deleted the feature-488-observability-topology branch October 26, 2022 14:51
@YianZong YianZong linked an issue Nov 24, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE]: CSM Operator: Support install of Observability
7 participants