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

mgr: failed to update the port of dashboard #11932

Merged
merged 1 commit into from
Apr 6, 2023
Merged

Conversation

zhucan
Copy link
Member

@zhucan zhucan commented Mar 20, 2023

Description of your changes:
Firstly we use the port 443 or 6443 to mgr dashboard, but it was used by other daemon; The dashboard will start failed and the status of the ceph will be HEALTH_ERR; then we update the port to a unused port,the operator can not deal with it. after this change, dashboard can start normally and the status of ceph will be HEALTH_OK.

Which issue is resolved by this Pull Request:
Resolves #11931

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide).
  • Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@zhucan zhucan changed the title mgr: failed to update the port of mgr's dashboard mgr: failed to update the port of dashboard Mar 20, 2023
@zhucan
Copy link
Member Author

zhucan commented Mar 20, 2023

releated qa: #11916 (comment)

Copy link
Contributor

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

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

@zhucan can you explain more about the changes in pr description? Thanks!

@zhucan
Copy link
Member Author

zhucan commented Mar 20, 2023

@zhucan can you explain more about the changes in pr description? Thanks!

Please take a look at the https://github.com/rook/rook/issues/11931, because of firstly we use the port 443 or 6443, but it was used by other daemon,the dashboard will start failed and the status of the ceph will be err,then we update the port to a unused port,the operator can not deal with it.

@travisn
Copy link
Member

travisn commented Mar 20, 2023

@zhucan Please also fill in the PR description section: Description of your changes:

@zhucan
Copy link
Member Author

zhucan commented Mar 21, 2023

@travisn done

pkg/operator/ceph/cluster/mgr/dashboard.go Outdated Show resolved Hide resolved
@@ -104,7 +100,13 @@ func (c *Cluster) configureDashboardModules() error {
}
if hasChanged {
logger.Info("dashboard config has changed. restarting the dashboard module")
return c.restartDashboard()
if err := c.restartDashboard(); 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.

Don't we need to restart the dashboard after the ssl cert is generated? Now we won't restart the dashboard if the ssl setting changes, won't that be a problem? Perhaps we need another dashboard restart inside initializeSecureDashboard() in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

hasChanged, err := c.initializeSecureDashboard()
The response is always false, if the configure of the dashboard not change, it will not restart under previous reality. we had tested with this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

other question is: after the ssl cert is generated, need to restart dashboard? we had tested with this case? may need to test the case.

Copy link
Member Author

Choose a reason for hiding this comment

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

@travisn please take a look at it

Copy link
Member

Choose a reason for hiding this comment

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

I see that the helper was always returning false, but the intention was that the dashboard should be restarted in case the ssl cert was generated. @jmolmo Does this sound correct, or do we not need to restart the dashboard module after creating the self signed cert?

pkg/operator/ceph/cluster/mgr/dashboard.go Show resolved Hide resolved
@@ -104,7 +100,13 @@ func (c *Cluster) configureDashboardModules() error {
}
if hasChanged {
logger.Info("dashboard config has changed. restarting the dashboard module")
return c.restartDashboard()
if err := c.restartDashboard(); 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.

I see that the helper was always returning false, but the intention was that the dashboard should be restarted in case the ssl cert was generated. @jmolmo Does this sound correct, or do we not need to restart the dashboard module after creating the self signed cert?

@mergify
Copy link

mergify bot commented Mar 31, 2023

This pull request has merge conflicts that must be resolved before it can be merged. @zhucan please rebase it. https://rook.io/docs/rook/latest/Contributing/development-flow/#updating-your-fork

@zhucan zhucan force-pushed the fix-11931 branch 3 times, most recently from f4d3a0d to 3cb8433 Compare April 3, 2023 09:49
pkg/operator/ceph/cluster/mgr/dashboard.go Outdated Show resolved Hide resolved
pkg/operator/ceph/cluster/mgr/dashboard.go Outdated Show resolved Hide resolved
Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

Actually, a fix is needed for the unit test TestStartSecureDashboard

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

Also please add one or two sentences to the commit description, which will help understand the changes in the git history. See the commit structure guide.

@travisn travisn added this to In progress in v1.11 via automation Apr 4, 2023
@travisn travisn moved this from In progress to Blocking Release in v1.11 Apr 4, 2023
if port was used by other daemon,the dashboard will start failed and the status of the ceph will be err,then we update the port to a unused port,the operator can not deal with it

Signed-off-by: zc <zc@zcdeMacBook-Pro.local>
@zhucan
Copy link
Member Author

zhucan commented Apr 6, 2023

@travisn please take a look

@travisn travisn merged commit 9f6923a into rook:master Apr 6, 2023
49 of 50 checks passed
v1.11 automation moved this from Blocking Release to Done Apr 6, 2023
travisn added a commit that referenced this pull request Apr 6, 2023
mgr: failed to update the port of dashboard (backport #11932)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v1.11
Done
Development

Successfully merging this pull request may close these issues.

mgr: update the port of mgr dashboard failed
3 participants