Remove calls to update vertica #39

spilchen · 2021-08-23T18:33:09Z

removed calls to update_vertica in the install and uninstall reconcile. This significantly speeds up scale-up operations.
new package (atconf) to handle modifying admintools.conf for install/uninstall
added a method to copy a file into a pod. It is implemented as a cat exec call.
had to explicitly accept the EULA now that we don't call update_vertica
had to explicitly check the existence and permissions of some directories in /opt/vertica/config (share and logrotate)
we pass the license in during the create_db command now that we don't call update_vertica anymore
had to change the obfuscation code to check for --password rather than -w. I added code that did a call to 'test -w file', and the obfuscation code was adding '****' for the file name.
moved ipv6 check function to its own package so that I can use it in the atconf package
moved name of vertica server container to names package. This was necessary so that we can use the const in the new atconf package

roypaulin · 2021-08-24T13:46:30Z

tests/e2e/crash-before-createdb/07-assert.yaml

@@ -1,14 +1,4 @@
 apiVersion: v1
-kind: Event


It is been a while for me so could you tell me why we do not need this anymore?

Because the install is so fast, we no longer need the Event for it. So all of the events for install and uninstall were removed.

roypaulin · 2021-08-24T14:03:00Z

docker-vertica/docker-entrypoint.sh

    rm -rf /home/dbadmin/logrotate

-    sudo mkdir -p /opt/vertica/config/licensing


Why don't we need sudo anymore?

We were over zealous with the sudo before and so it wasn't needed. The long term goal is to have no reliance on sudo as that will play nice when we support OpenShift. Removing calls to update_vertica was a key first step for that.

Ok now I get it thanks.

ningdeng · 2021-08-24T17:49:28Z

pkg/atconf/file_writer.go

+	}
+}
+
+// AddHosts will had ips to an admintools.conf.  New admintools.conf, stored in


minor typo: had --> add.

I'm trying to get more details: is it only adding IPs to the hosts = entry in [Cluster] section?

Edit: nvm, found it's adding IPs to both hosts= entry and creating new compact node name entries in [Nodes] section.

Also, does it check duplicate IPs?

Edit: nvm, upon looking at the called functions I think my questions are answered.

That's right. We handle duplicate IPs by treating it as a no-op. I will update the comment here.

ningdeng · 2021-08-24T18:14:52Z

pkg/atconf/file_writer.go

+	return nil
+}
+
+// removeNodes will remove the nodes section for the given set of IPs


I was wondering if this function is expected to remove only compat21 node entries, if yes, should we assert or check somewhere to make sure we do not touch regular database nodes, at least mention in the comment? I understand those regular database nodes will be removed and are supposed to be removed by db_remove_subcluster/db_remove_nodes before the uninstallation.

That's right. We only remove compat21 node entries. The AT -t db_remove_nodes will handle removing of regular database nodes. I will mention in a comment.

ningdeng · 2021-08-24T18:27:34Z

pkg/controllers/at.go

+		if !p.isPodRunning {
+			continue
+		}
+		_, _, err := pr.CopyToPod(ctx, p.name, names.ServerContainer, atConfTempFile, paths.AdminToolsConf)


Is it possible that the copying succeed on some Pods but fail on other Pods?

Yes it is possible. We make sure that all pods are running before we begin install/uninstall, but that just shortens the timing. Do you think it is necessary to have a step in the operator to ensure admintools.conf is the same on all of the pods? And copy if they are different. The same pod is always picked first, so it should be a well known location that has the correct version of admintools.conf.

Do you think it is necessary to have a step in the operator to ensure admintools.conf is the same on all of the pods?
-- Having a step regularly checking admintools.conf on all Pods may be overkilling, as long as the initiator of AT commands has the latest admintools.conf it should be ok. Maybe we can keep as is at this time, if tests show any problem, then we can see what we can do.

I'm not sure about the details of err message, just want to mention that it might be helpful if we know on which nodes the copy succeeded and on which the copy failed, so if there's at least one copy succeeded, then that copy can be the source of latest admintools.conf, such information could be helpful to help debugging and fix admintools.conf

@ningdeng based on your comments here, I improved the distributeAdmintoolsConf function.

the first pod we copy to will always be the same. It is also the pod that we use for the base for any subsequent admintools.conf changes for install/uninstall

if the first pod copied was okay, copy to all other pods. We save the errors and do checking at the end so that we attempt to copy to all pods

if the copy failed at one of the pods, log an event saying that admintools.conf was partially copied.

You can see these changes in the commit c424386

ningdeng · 2021-08-24T19:10:07Z

This looks good to me overall, thanks!

ningdeng · 2021-08-24T20:52:30Z

pkg/controllers/at.go

+	// Copy the admintools.conf to the rest of the pods.  We will do error
+	// checking at the end so that we try to copy it to each pod.
+	errs := []error{}
+	for _, p := range pf.Detail {


The latest improvements of copying admintools.conf change looks good to me. Thanks!

Just some minor notes as potential future improvements: I see this is a sequential operation as it uses a for-loop, starting from v11, AT uses a new file copying mechanism, which improves the performance of file distribution to the entire cluster a lot. So, just in case that in the future people suggest/request updating this copying to be a parallel operation on a very large cluster, AT distribute_config_files could be used to distribute the files. At this time, using CopyToPod is better IMO.

Okay, thanks. This is good to know and I will keep it in mind.

Matt Spilchen added 13 commits August 6, 2021 10:27

Phase 1 of removing calls to update_vertica

86490ba

Merge branch 'main' into remove-update-vertica

7d417ee

Code cleanup for install

f834627

Fixes for e2e failures

5f4ee81

Allow host removal from atconf

c9ba794

Avoid calling update_vertica during uninstall

ae698eb

Increase test coverage

e3e0a74

Return pod name when calling anyPodsNotRunning

043866b

Code cleanup

d5f6059

Fix testcase

d3ffbf1

Merge branch 'main' into remove-update-vertica

eb70722

Merge branch 'main' into remove-update-vertica

a53b832

Add changie

04322dc

spilchen requested review from ningdeng and roypaulin August 23, 2021 18:33

spilchen self-assigned this Aug 23, 2021

Fix kill-controller test

dfe0d99

roypaulin reviewed Aug 24, 2021

View reviewed changes

ningdeng reviewed Aug 24, 2021

View reviewed changes

Matt Spilchen added 2 commits August 24, 2021 15:00

Apply review comments

1d6f814

Merge branch 'main' into remove-update-vertica

56d443f

ningdeng approved these changes Aug 24, 2021

View reviewed changes

roypaulin approved these changes Aug 24, 2021

View reviewed changes

Improve at copy function

c424386

ningdeng reviewed Aug 24, 2021

View reviewed changes

spilchen merged commit 027ff64 into vertica:main Aug 25, 2021

spilchen deleted the remove-update-vertica branch August 25, 2021 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove calls to update vertica #39

Remove calls to update vertica #39

spilchen commented Aug 23, 2021

roypaulin Aug 24, 2021

spilchen Aug 24, 2021

roypaulin Aug 24, 2021

spilchen Aug 24, 2021

roypaulin Aug 24, 2021

ningdeng Aug 24, 2021 •

edited

Loading

ningdeng Aug 24, 2021

ningdeng Aug 24, 2021

spilchen Aug 24, 2021

ningdeng Aug 24, 2021

spilchen Aug 24, 2021

ningdeng Aug 24, 2021

spilchen Aug 24, 2021

ningdeng Aug 24, 2021

ningdeng Aug 24, 2021

spilchen Aug 24, 2021

ningdeng commented Aug 24, 2021

ningdeng Aug 24, 2021

spilchen Aug 24, 2021

		rm -rf /home/dbadmin/logrotate

		sudo mkdir -p /opt/vertica/config/licensing

Remove calls to update vertica #39

Remove calls to update vertica #39

Conversation

spilchen commented Aug 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningdeng Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningdeng commented Aug 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningdeng Aug 24, 2021 •

edited

Loading