Skip to content

mgr/cephadm: Prevent unnecessary SSH reconfiguration#36492

Merged
sebastian-philipp merged 1 commit intoceph:masterfrom
ricardoasmarques:prevent-unnecessary-ssh-reconfiguration
Aug 7, 2020
Merged

mgr/cephadm: Prevent unnecessary SSH reconfiguration#36492
sebastian-philipp merged 1 commit intoceph:masterfrom
ricardoasmarques:prevent-unnecessary-ssh-reconfiguration

Conversation

@ricardoasmarques
Copy link
Contributor

@ricardoasmarques ricardoasmarques commented Aug 6, 2020

When set-user, set-pub-key or set-priv-key are called "two" times with the same value, we can ignore the "second" call:

# ceph cephadm set-pub-key -i pub_key 
# ceph cephadm set-pub-key -i pub_key 
value already set

This will prevent unnecessary SSH reconfiguration and connections reset.

Signed-off-by: Ricardo Marques rimarques@suse.com

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@ricardoasmarques ricardoasmarques requested a review from a team as a code owner August 6, 2020 10:01
@ricardoasmarques ricardoasmarques mentioned this pull request Aug 6, 2020
Closed
tchaikov
tchaikov previously requested changes Aug 6, 2020
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes

@ricardoasmarques ricardoasmarques force-pushed the prevent-unnecessary-ssh-reconfiguration branch from 3349dfd to 00e71d3 Compare August 6, 2020 12:19
@ricardoasmarques ricardoasmarques changed the title Prevent unnecessary SSH reconfiguration mgr/cephadm: Prevent unnecessary SSH reconfiguration Aug 6, 2020
@ricardoasmarques
Copy link
Contributor Author

ricardoasmarques commented Aug 6, 2020

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes

Fixed, thanks.

mgfritch
mgfritch reviewed Aug 6, 2020
View reviewed changes
Copy link
Contributor

@mgfritch mgfritch left a comment

Choose a reason for hiding this comment

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

also extend this to set-ssh-config?

ceph/src/pybind/mgr/cephadm/module.py

Lines 742 to 746 in 9809c18

if inbuf is None or len(inbuf) == 0:
return -errno.EINVAL, "", "empty ssh config provided"
self.set_store("ssh_config", inbuf)
self.log.info('Set ssh_config')
self._reconfig_ssh()

@ricardoasmarques
mgr/cephadm: Prevent unnecessary SSH reconfiguration
4cbf587 
When 'set-user', 'set-pub-key' or 'set-priv-key' are called two
times with the same value, we can ignore the second call.

Signed-off-by: Ricardo Marques <rimarques@suse.com>
@ricardoasmarques ricardoasmarques force-pushed the prevent-unnecessary-ssh-reconfiguration branch from 00e71d3 to 4cbf587 Compare August 6, 2020 15:53
@ricardoasmarques
Copy link
Contributor Author

ricardoasmarques commented Aug 6, 2020

also extend this to set-ssh-config?

ceph/src/pybind/mgr/cephadm/module.py

Lines 742 to 746 in 9809c18

if inbuf is None or len(inbuf) == 0:
return -errno.EINVAL, "", "empty ssh config provided"
self.set_store("ssh_config", inbuf)
self.log.info('Set ssh_config')
self._reconfig_ssh()

@mgfritch I've implemented the same behaviour on set-ssh-config.

I've also changed the message to make it more clear that SSH is not reconfigured because the value has not changed.

@tchaikov tchaikov dismissed their stale review August 6, 2020 15:56

dismissed

@sebastian-philipp
Copy link
Contributor

sebastian-philipp commented Aug 6, 2020

mgfritch
mgfritch approved these changes Aug 6, 2020
View reviewed changes
Copy link
Contributor

@mgfritch mgfritch left a comment

Choose a reason for hiding this comment

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

lgtm!

src/pybind/mgr/cephadm/module.py
if ssh_config_fname:
self.validate_ssh_config_fname(ssh_config_fname)
ssh_options += ['-F', ssh_config_fname]
self.ssh_config = ssh_config
Copy link
Contributor

@mgfritch mgfritch Aug 6, 2020

Choose a reason for hiding this comment

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

👍

@sebastian-philipp sebastian-philipp added the wip-swagner-testing My Teuthology tests label Aug 7, 2020
@sebastian-philipp
Copy link
Contributor

sebastian-philipp commented Aug 7, 2020

https://pulpito.ceph.com/swagner-2020-08-07_14:25:21-rados:cephadm-wip-swagner-testing-2020-08-07-1340-distro-basic-smithi/

@sebastian-philipp sebastian-philipp merged commit bcea5cc into ceph:master Aug 7, 2020
@sebastian-philipp sebastian-philipp mentioned this pull request Aug 21, 2020
Merged
@snyk-bot snyk-bot mentioned this pull request May 8, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgfritch mgfritch mgfritch approved these changes

@tchaikov tchaikov tchaikov left review comments

+1 more reviewer

@sebastian-philipp sebastian-philipp sebastian-philipp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cephadm needs-qa wip-swagner-testing My Teuthology tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ricardoasmarques @sebastian-philipp @tchaikov @mgfritch