Adding ceph-salt task and suite #373

gekios · 2020-03-12T13:03:16Z

Adding a ceph-salt task and suite and also having suse suite point at it instead of deepsea so it runs on our CI.

kshtsk · 2020-03-12T14:02:13Z

qa/tasks/scripts/install_ceph_salt.sh

+	if [ ! -n "$BRANCH" ]
+	then
+		BRANCH="master"
+	fi


This can be replaced with single command:

BRANCH=${BRANCH:-"master"}

And in fact, it is better to move it outside of the if when you declare this variable:

BRANCH=${2:-"master"}

kshtsk · 2020-03-12T14:04:20Z

qa/tasks/scripts/install_ceph_salt.sh

+	then
+		BRANCH="master"
+	fi
+	cd /root


Why do we need to go to /root?

I had just copied the code from sesdev but I don't see any particular reason to clone the repo in /root dir so I removed it and testing it to be sure

qa/tasks/scripts/install_ceph_salt.sh

kshtsk · 2020-03-12T14:11:03Z

qa/tasks/ceph_salt.py

+        self.ctx.cluster.run(args=[ 'sudo', 'sh', '-c',
+                                    'systemctl restart salt-minion'])


'sh', '-c' are not required, you can use just:
self.ctx.cluster.run(args='sudo systemctl restart salt-minion')

kshtsk · 2020-03-12T14:12:30Z

qa/tasks/ceph_salt.py

+        self.ctx.cluster.run(args=[ 'sudo', 'sh', '-c', 
+                                    'sudo zypper -n addrepo --refresh --no-gpgcheck'
+                                    ' http://download.suse.de/ibs/SUSE:/CA/SLE_15_SP2/ '
+                                    '\'SUSE Internal CA Certificate\'; '
+                                    'zypper -n in ca-certificates-suse'])


why do you need 'sudo' two times?

for improving readability and maintainability can this code be refactored like:

self.ctx.cluster.run(args=""" sudo zypper -n addrepo --refresh --no-gpgcheck http://download.suse.de/ibs/SUSE:/CA/SLE_15_SP2/ 'SUSE Internal CA Certificate' sudo zypper -n in ca-certificates-suse""")

or

repo_url = 'http://download.suse.de/ibs/SUSE:/CA/SLE_15_SP2/' repo_name = "SUSE Internal CA Certificate" self.ctx.cluster.run(args=" sudo zypper -n addrepo --refresh --no-gpgcheck {url} '{name}' sudo zypper -n in ca-certificates-suse".format(url=repo_url, name=repo_name))

or something else which will be easier to read.

kshtsk · 2020-03-12T14:15:44Z

qa/tasks/ceph_salt.py

+        self.ctx.cluster.run(args=[ 'sudo', 'sh', '-c',
+                                    'cat /etc/hosts' ])
+        self.ctx.cluster.run(args=[ 'sudo', 'sh', '-c',
+                                    'chattr +i /etc/hosts'])


Usually, 'sudo sh -c' is not required, it is only required when you want to combine calls or process specifically exit codes, so just use: cluster.run(args='sudo cat /etc/hosts') or cluster.run(args='sudo chattr +i /etc/hosts')

kshtsk · 2020-03-12T14:15:58Z

qa/tasks/ceph_salt.py

+        '/etc/hosts' in all hosts in order to use DNS server for names resolution.
+        '''
+        self.log.info(anchored("Removing localhost resolution from /etc/hosts"))
+        self.ctx.cluster.run(args=[ 'sudo', 'sh', '-c', 


'sh -c' is not required here.

kshtsk · 2020-03-12T14:20:59Z

qa/tasks/ceph_salt.py

+        causes salt to populate 'fqdn_ip4' with 127.0.0.1 and ceph-salt is breaking since it
+        has a exception for localhost resolution. So this function Removes localhost entries from
+        '/etc/hosts' in all hosts in order to use DNS server for names resolution.
+        '''


I'm not sure this helps to make salt use DNS for hostname resolution, have checked a possibility to give ceph-salt predefined ip addresses?

It's not available yet on ceph-salt. See ceph/ceph-salt#64

The following PR will allow the user to explicitly set bootstrap Mon IP address: ceph/ceph-salt#156

kshtsk · 2020-03-12T14:21:24Z

qa/tasks/ceph_salt.py

+        has a exception for localhost resolution. So this function Removes localhost entries from
+        '/etc/hosts' in all hosts in order to use DNS server for names resolution.
+        '''
+        self.log.info(anchored("Removing localhost resolution from /etc/hosts"))


This is basically a workaround, we need to declare this some how.

Added #FIXME to point it out

qa/tasks/scripts/install_ceph_salt.sh

qa/tasks/salt_manager.py

kshtsk · 2020-03-12T14:24:39Z

qa/tasks/salt_manager.py

+        List updates and perform update if needed
+        """
+        zypper_lu = "sudo salt \\* cmd.run \'zypper lu || true\' 2>/dev/null"
+        zypper_up = "zypper -n up"


will this run successfully even without sudo?

No you are right my bad I missed it.

kshtsk · 2020-03-12T14:34:49Z

qa/tasks/ceph_salt.py

+            self.master_remote.sh("sudo ceph-salt config /Cluster/Minions add {}".format(host))
+            for role in roles:
+                role = role.split('.')[0]
+                if role in ceph_salt_roles.keys():


You do not need to get use .keys() to check if a dictionary has it in python, here is more pythonish style:

if role in ceph_salt_roles: cluster_role = '/Cluster/Roles/' + ceph_salt_roles[role] self.master_remote.sh("sudo ceph-salt config {} add {}".format(cluster_role, host))

OK I aggree with you on the .keys part but I think it's more readable to have the whole command instead of having cluster_role

for python dev it should be readable ;-)

toabctl · 2020-03-12T15:21:53Z

ok to test

kshtsk · 2020-03-18T15:01:04Z

resolve conflict required

gekios · 2020-06-09T11:01:04Z

Running a test to be sure I haven't broken anything. I reverted the salt_master role change and added the role parameter in the constructor

gekios · 2020-06-09T11:10:45Z

OK it passed. http://teuth-ses.prv.suse.net:8081/runner-2020-06-09_10:49:12-suse-wip-ses7-ceph-salt-none-basic-ecp/

kshtsk

lgtm

smithfarm · 2020-06-09T15:40:52Z

qa/ceph_salt/ceph_salt.yaml

+tasks:
+        - ceph_salt:
+                repo: https://github.com/ceph/ceph-salt.git
+                branch: master


for the CI, we should be installing ceph-salt from RPM, not from source...

smithfarm · 2020-06-09T15:45:44Z

qa/ceph_salt/salt.yaml

@@ -2,5 +2,5 @@ tasks:
 - clock:
 - install:
    install_ceph_packages: false
-    extra_system_packages: [salt, salt-master, salt-minion, salt-api]
+    extra_system_packages: [salt, salt-master, salt-minion, salt-api, which, lsof]


what is which needed for? I tend to avoid it like the plague.

iirc it's is needed for ceph-salt and it was missing from the image. I will test if it works without it and remove if if it's not needed anymore.

not needed for ceph-salt since ceph/ceph-salt#85 was merged

smithfarm · 2020-06-09T15:53:43Z

qa/tasks/ceph_salt.py

+            self.scripts.run(
+            self.master_remote,
+            'install_ceph_salt.sh'
+            )


smithfarm · 2020-06-09T15:53:49Z

qa/tasks/ceph_salt.py

+                self.master_remote,
+                'install_ceph_salt.sh',
+                args=[ceph_salt_ctx['repo'], ceph_salt_ctx['branch']]
+                )


smithfarm · 2020-06-09T15:53:58Z

qa/tasks/ceph_salt.py

+                self.master_remote,
+                'install_ceph_salt.sh',
+                args=ceph_salt_ctx['repo']
+                )


smithfarm · 2020-06-09T15:54:55Z

qa/tasks/ceph_salt.py

+            logger=log.getChild('mon.' + first_mon),
+            wait=False,
+            started=True,
+            )


smithfarm · 2020-06-09T15:55:03Z

qa/tasks/ceph_salt.py

+            logger=log.getChild('mgr.' + first_mgr),
+            wait=False,
+            started=True,
+            )


smithfarm · 2020-06-09T16:13:06Z

qa/tasks/salt_manager.py

+    :param ctx:     Context from the main task
+    :param role:    role that will be used as Salt Master (default 'client.salt_master')
+    """
+    def __init__(self, ctx, role = master_role):


please, without the spaces: role=master_role

gekios · 2020-06-09T17:06:41Z

I was running tox that has flake8 but wasn't finding the errors for some reason. I fixed all of them. It's clean

smithfarm · 2020-06-10T06:59:06Z

I was running tox that has flake8 but wasn't finding the errors for some reason. I fixed all of them. It's clean

Thanks. Please remove the which and then I will merge it.

Adding Ceph-salt task and a basic suite under qa/suites/ceph_salt. Also changed suse suite to point to ceph-salt instead of deepsea. Signed-off-by: Georgios Kyratsas <gkyratsas@suse.com>

Adding currently unhandled NoValidConnectionsError exception Signed-off-by: Georgios Kyratsas <gkyratsas@suse.com>

Removing all deepsea related files from ses7. Signed-off-by: Georgios Kyratsas <gkyratsas@suse.com>

Adding as optional parameter in the constructor the teuthology role that will be used as the salt master. Signed-off-by: Georgios Kyratsas <gkyratsas@suse.com>

smithfarm

Nice work!

gekios requested review from toabctl, sebastian-philipp, smithfarm and kshtsk March 12, 2020 13:03

gekios mentioned this pull request Mar 12, 2020

Adding ceph-salt task and suite for ses7 CI #371

Closed

gekios force-pushed the wip-ses7-ceph-salt branch from 7a99b58 to 9664fdf Compare March 12, 2020 13:07

kshtsk reviewed Mar 12, 2020

View reviewed changes

qa/tasks/scripts/install_ceph_salt.sh Show resolved Hide resolved

kshtsk reviewed Mar 12, 2020

View reviewed changes

qa/tasks/scripts/install_ceph_salt.sh Show resolved Hide resolved

kshtsk reviewed Mar 12, 2020

View reviewed changes

qa/tasks/salt_manager.py Show resolved Hide resolved

kshtsk reviewed Mar 12, 2020

View reviewed changes

smithfarm force-pushed the ses7 branch from 3d31471 to 310e512 Compare March 13, 2020 10:16

sebastian-philipp added the needs-rebase label Mar 13, 2020

gekios force-pushed the wip-ses7-ceph-salt branch 7 times, most recently from 54fe89a to b3d1075 Compare March 19, 2020 12:47

gekios force-pushed the wip-ses7-ceph-salt branch 2 times, most recently from 9e9f93f to ea7c914 Compare June 9, 2020 10:56

gekios force-pushed the wip-ses7-ceph-salt branch from ea7c914 to 977c509 Compare June 9, 2020 11:41

kshtsk approved these changes Jun 9, 2020

View reviewed changes