always log output to a file #1205

aspiers · 2016-09-02T15:38:56Z

This eliminates the risk of losing crucial debug output in a long mkcloud run because you forgot to redirect to a file and the terminal/screen/tmux scrollback buffer wasn't big enough.

Closes #1191.

dirkmueller · 2016-09-02T15:51:35Z

I'm not sure that I would want the logfiles to be in /var/log rather than the current scratchdir but it LGTM, +1

aspiers · 2016-09-02T17:05:01Z

@dirkmueller Thanks. Regarding the location, your opinions are very welcome on the discussion in #224 ... as long as you agree with me. Just kidding ;-)

jdsn · 2016-09-02T20:26:11Z

scripts/mkcloud

@@ -15,6 +15,13 @@
 # 2. This tool relies on the script qa_crowbarsetup.sh
 # 3. Please 'export' environment variables according to your needs.

+: ${virtualcloud:=virtual}
+
+: ${log_dir:=/var/log/mkcloud/$virtualcloud}


👎
Oh there can be many clouds with the (internal) name 'virtual' on the same host.
The differentiating name is held in the $cloud variable.

nicolasbock · 2016-09-03T03:23:04Z

+1

jdsn · 2016-09-05T10:07:59Z

-100 - see commented code

aspiers · 2016-09-05T10:34:01Z

OK, I think I see what you mean. This would break things for non-virtual clouds like the hardware QA clouds? I find the use of these undocumented variables in the code (including untracked scripts on mkch* hosts) very confusing.

Tangent: IMHO this just proves the importance of #220 and #256, which sadly have not progressed in over a year, because currently noone owns the responsibility of fixing them. If we clearly defined ownership / maintainership for the various components, I expect this type of problem of lack of progress would happen less. It would still happen of course because we all have too much to do and not enough time, but ownership -> specialization -> focus -> less context-switching / fewer distractions -> less tired brains -> more productivity!

jdsn · 2016-09-05T10:46:28Z

This would break things for non-virtual clouds like the hardware QA clouds?

@aspiers: No, thats not the point. You can run multiple 'virtual' clouds on a single host at the same time. With this proposed patch they would both log to the same log file. Thats the conflict.

Instead of $virtualcloud just use $cloud and the patch would not create a conflict.
Because mkcloud differes between clouds by this variable.
Or put otherwise: if you start the second cloud with the name cloud=cloudABC then it will cleanup the first one during cleanup step.

I find the use of these undocumented variables in the code (including untracked scripts on mkch* hosts)

There should no longer be untracked scripts. We put them into git even months ago.

very confusing.

Thats why I am looking forward to join the automation squad soon, to be able to improve and cleanup a lot.

aspiers · 2016-09-05T11:06:51Z

This would break things for non-virtual clouds like the hardware QA clouds?

@aspiers: No, thats not the point. You can run multiple 'virtual' clouds on a single host at the same time. With this proposed patch they would both log to the same log file. Thats the conflict.

No they wouldn't. For example, the mysteriously-named runtestn in scripts/mkcloudhost/ has

export virtualcloud=$vcloudname$n
export cloud=$virtualcloud

so in the majority of cases, both variables have the same value which is different for each virtual cloud on a host. (Secondly, this PR makes each mkcloud invocation log to a separate time-stamped file anyway, so no two mkcloud invocations would ever log to the same file, even if we always set virtualcloud to the same value.)

Instead of $virtualcloud just use $cloud and the patch would not create a conflict.
Because mkcloud differes between cloud by this variable.

Please can you explain how this can possibly be true given the above code? Because I just don't get it. Having said that, I have no problem changing the PR to use $cloud but I just can't see what difference it would make in the majority of situations.

Or put otherwise: if you start the second cloud with the name cloud=cloudABC then it will cleanup the first one during cleanup step.

Sorry, I don't understand this sentence at all. What is cloudABC in this scenario and how does it relate to the first cloud?

I find the use of these undocumented variables in the code (including untracked scripts on mkch* hosts)

There should no longer be untracked scripts. We put them into git even months ago.

There are still loads of untracked scripts, because I'm not just talking about the mkch* host config, e.g.

mkcloud:~ # for d in manual.*; do ([ -d $d ] && cd $d && git status); done 2>&1 | grep -c "Not a git repo"
26

But even the host config had issues last time I looked.

Thats why I am looking forward to join the automation squad soon, to be able to improve and cleanup a lot.

Sounds great!

jdsn · 2016-09-05T11:36:04Z

Please can you explain how this can possibly be true given the above code?

Sure. There is more than just our mkcloud workers who happen to set different names even for the virtual clouds. Everybody can run multiple clouds on his own system/workstation/server/wherever and they will all default to the same virtualcloud name "virtual". This value is just used to create different network configurations in qa_crowbarsetup. And "virtual" is just one case (besides d1, d2,.., qa1, qa2,...,cumulus).

And here is the default:
grep virtualcloud mkcloud | head -n 1
So its totally fine to have many 'virtual' clouds on one host.

I have no problem changing the PR to use $cloud

Ok, thanks, please change it :)

What is cloudABC in this scenario and how does it relate to the first cloud?

It was an example.
Assume you already have a cloud called export cloud=cloudABC. And now you start a second one with the same name export cloud=cloudABC. Then the first one will be cleaned up. With this example I wanted to show that the differentiating variable is $cloud and not $virtualcloud.

... I just can't see what difference it would make in the majority of situations

Well the majority is not enough for this kind of change. It should break no other use cases.

mkcloud:~ # for d in manual.* ...

Its called manual .... It has nothing to do with jenkins or any official part of our automation. These are configs from developers for manually created clouds which they use temporarily. Its just developers playing with clouds. Its kind of their home directory on an mkcloud host.
It should be their decision if they want to track their configs in git or not.

You could also name such a directory
mkdir adams_mkcloud_playground_please_do_not_touch_and_I_wont_touch_yours

aspiers · 2016-09-05T12:42:54Z

Please can you explain how this can possibly be true given the above code?

Sure. There is more than just our mkcloud workers who happen to set different names even for the virtual clouds. Everybody can run multiple clouds on his own system/workstation/server/wherever and they will all default to the same virtualcloud name "virtual".

Although AFAICS that default value never gets used, because in the virtual cloud cases, the variable gets set to same value as $cloud, e.g. something like c19. And this only works because there is a * catch-all case in setcloudnetvars which behaves identically to the virtual case:

This value is just used to create different network configurations in qa_crowbarsetup.

OK, now it's becoming clearer. Actually this value is not directly referenced in qa_crowbarsetup.sh at all, but it's passed to it via this line in mkcloud:

setcloudnetvars $virtualcloud

And "virtual" is just one case (besides d1, d2,.., qa1, qa2,...,cumulus).

OK, so $virtualcloud is actually a misnomer and it should be called something like $cloudhardware. It makes no sense for a variable called $virtualcloud to have a value which refers to a non-virtual cloud.

So its totally fine to have many 'virtual' clouds on one host.

As the person who has complained the most about resource contention on mkcloud1, I am already painfully aware of that ...

I have no problem changing the PR to use $cloud

Ok, thanks, please change it :)

Will do. But we should also improve the variable naming ...

What is cloudABC in this scenario and how does it relate to the first cloud?

It was an example.
Assume you already have a cloud called export cloud=cloudABC. And now you start a second one with the same name export cloud=cloudABC. Then the first one will be cleaned up. With this example I wanted to show that the differentiating variable is $cloud and not $virtualcloud.

... I just can't see what difference it would make in the majority of situations

Well the majority is not enough for this kind of change. It should break no other use cases.

Agreed - I was trying to understand which situations it would break, and it seems I confirmed my guess that it would break the hardware clouds where mkcloud is used (but not the QA clouds where for some reason they still run qa_crowbarsetup.sh directly).

mkcloud:~ # for d in manual.* ...

Its called manual .... It has nothing to do with jenkins or any official part of our automation.
These are configs from developers for manually created clouds which they use temporarily. Its just developers playing with clouds. Its kind of their home directory on an mkcloud host.

I know that, of course! That's why I said "I find the use of these undocumented variables in the code (including untracked scripts on mkch* hosts) very confusing", and also why I posted this:

http://mailman.suse.de/mlarch/SuSE/cloud-devel/2016/cloud-devel.2016.08/msg00185.html

It should be their decision if they want to track their configs in git or not.

You could also name such a directory
mkdir adams_mkcloud_playground_please_do_not_touch_and_I_wont_touch_yours

I think that's an overly narrow view. To make this stuff all work well, we need to consider use cases, how developers are currently using mkcloud, and how we would like to use it. Currently we know there are several gaps in functionality which have lead to the ugly mess of /root/manual.*, and fixing the design as per the above mail will help eliminate those. But let's keep that discussion on cloud-devel not in this github issue ...

vuntz · 2016-09-05T14:56:06Z

+1

aspiers · 2016-09-05T15:46:50Z

Now uses $cloud instead of $virtualcloud - please re-review.

jdsn · 2016-09-05T15:53:04Z

... that default value never gets used, ... something like c19

Again, there is more than our mkchX workers. The default will be default for all other runs.

but it's passed to it via this line in mkcloud:
setcloudnetvars $virtualcloud

No, it is passed to qa_crowbarsetup with this line:
export cloud=$virtualcloud
Note: this line is created on the outside host but is executed on the crowbar node.
Historical naming led to inconsistency.

... so $virtualcloud is actually a misnomer and it should be called something like $cloudhardware

If we want to call kvm virtual 'hardware', yes :)

It makes no sense for a variable called $virtualcloud to have a value which refers to a non-virtual cloud.

Thats historical debt. In the beginning this variable was kind of a boolean. It only switched between virtual and non virtual. The support for other clouds (real hardware clouds) was introduced later and the same variable was (mis)used without refactoring it (I guess because we did not want to break existing automation back then).

IMHO configuration data for different hardware clouds should not even be part of mkcloud/qa_crowbarsetup but rather be kept in separately maintained config files.
Thats another thing I'd like to clean up.

jdsn · 2016-09-05T15:53:23Z

Now uses $cloud instead of $virtualcloud - please re-review.

Thanks

👍

vuntz · 2016-09-06T07:32:04Z

Travis is not happy :/

bmwiedemann · 2016-09-06T08:57:48Z

scripts/mkcloud

+for required_var in cloud; do
+    if [ "${!required_var:+defined}" != 'defined' ]; then
+        echo "mkcloud requires $required_var variable to be set; aborting" >&2
+        exit 1


-1 "mkcloud requires cloud variable to be set" breaks mkcloud help function as discovered by travis CI test.

Oops, good point :-/

aspiers · 2016-09-06T12:56:54Z

OK, third time lucky.

jdsn · 2016-09-07T09:11:15Z

scripts/mkcloud

@@ -15,6 +15,15 @@
 # 2. This tool relies on the script qa_crowbarsetup.sh
 # 3. Please 'export' environment variables according to your needs.

+if [[ "$cloud" ]]; then


Why not move this line
: ${cloud:=cloud}
above this block and skip the check?

Btw: no quotes required.

I agree. There is no point in testing for a variable, that gets a default value some lines below.

This eliminates the risk of losing crucial debug output in a long mkcloud run because you forgot to redirect to a file and the terminal/screen/tmux scrollback buffer wasn't big enough. Closes SUSE-Cloud#1191.

aspiers · 2016-09-07T13:08:21Z

Fourth time lucky ...

jdsn · 2016-09-07T13:58:38Z

still +1 if gating passes

dirkmueller · 2016-09-07T14:43:37Z

+1

aspiers assigned vuntz, jdsn and bmwiedemann Sep 2, 2016

aspiers mentioned this pull request Sep 2, 2016

mkcloud writes temporary files in cwd #224

Open

jdsn reviewed Sep 2, 2016
View reviewed changes

aspiers force-pushed the always-log branch 2 times, most recently from 025d2b1 to f8ada33 Compare September 5, 2016 12:52

aspiers force-pushed the always-log branch from f8ada33 to ed43e3a Compare September 5, 2016 15:44

bmwiedemann reviewed Sep 6, 2016
View reviewed changes

aspiers force-pushed the always-log branch from ed43e3a to f9d0b07 Compare September 6, 2016 12:39

aspiers force-pushed the always-log branch from f9d0b07 to 45aeee0 Compare September 6, 2016 12:59

jdsn reviewed Sep 7, 2016
View reviewed changes

always log output to a file

2a4f415

This eliminates the risk of losing crucial debug output in a long mkcloud run because you forgot to redirect to a file and the terminal/screen/tmux scrollback buffer wasn't big enough. Closes SUSE-Cloud#1191.

aspiers force-pushed the always-log branch from 45aeee0 to 2a4f415 Compare September 7, 2016 12:22

dirkmueller merged commit 6122738 into SUSE-Cloud:master Sep 7, 2016

aspiers deleted the always-log branch April 4, 2017 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

always log output to a file #1205

always log output to a file #1205

aspiers commented Sep 2, 2016

dirkmueller commented Sep 2, 2016 •

edited

Loading

aspiers commented Sep 2, 2016 •

edited

Loading

jdsn Sep 2, 2016 •

edited

Loading

nicolasbock commented Sep 3, 2016

jdsn commented Sep 5, 2016

aspiers commented Sep 5, 2016

jdsn commented Sep 5, 2016 •

edited

Loading

aspiers commented Sep 5, 2016

jdsn commented Sep 5, 2016

aspiers commented Sep 5, 2016

vuntz commented Sep 5, 2016

aspiers commented Sep 5, 2016

jdsn commented Sep 5, 2016 •

edited

Loading

jdsn commented Sep 5, 2016

vuntz commented Sep 6, 2016

bmwiedemann Sep 6, 2016

aspiers Sep 6, 2016

aspiers commented Sep 6, 2016

jdsn Sep 7, 2016

bmwiedemann Sep 7, 2016

aspiers commented Sep 7, 2016

jdsn commented Sep 7, 2016

dirkmueller commented Sep 7, 2016

always log output to a file #1205

always log output to a file #1205

Conversation

aspiers commented Sep 2, 2016

dirkmueller commented Sep 2, 2016 • edited Loading

aspiers commented Sep 2, 2016 • edited Loading

jdsn Sep 2, 2016 • edited Loading

Choose a reason for hiding this comment

nicolasbock commented Sep 3, 2016

jdsn commented Sep 5, 2016

aspiers commented Sep 5, 2016

jdsn commented Sep 5, 2016 • edited Loading

aspiers commented Sep 5, 2016

jdsn commented Sep 5, 2016

aspiers commented Sep 5, 2016

vuntz commented Sep 5, 2016

aspiers commented Sep 5, 2016

jdsn commented Sep 5, 2016 • edited Loading

jdsn commented Sep 5, 2016

vuntz commented Sep 6, 2016

bmwiedemann Sep 6, 2016

Choose a reason for hiding this comment

aspiers Sep 6, 2016

Choose a reason for hiding this comment

aspiers commented Sep 6, 2016

jdsn Sep 7, 2016

Choose a reason for hiding this comment

bmwiedemann Sep 7, 2016

Choose a reason for hiding this comment

aspiers commented Sep 7, 2016

jdsn commented Sep 7, 2016

dirkmueller commented Sep 7, 2016

dirkmueller commented Sep 2, 2016 •

edited

Loading

aspiers commented Sep 2, 2016 •

edited

Loading

jdsn Sep 2, 2016 •

edited

Loading

jdsn commented Sep 5, 2016 •

edited

Loading

jdsn commented Sep 5, 2016 •

edited

Loading