quattor / ncm-ncd

Node Configuration Dispatcher Framework for Components

www.quattor.org

Other

4 stars 9 forks source link

ncm-ncd: run from temporary directory #123

Closed stdweird closed 6 years ago

stdweird commented 6 years ago

Replaces #121

stdweird commented 6 years ago

@ttyS4 i haven't tested this, and i still need to add unittests

stdweird commented 6 years ago

tested without chroot @ned21 @ttyS4 does MS use chroot?

ttyS4 commented 6 years ago

No, we dont use the chroot feature (just confirmed with someone here).

ttyS4 commented 6 years ago

As far as I see you use the internal directory function here. One thing which made me not use it is a confusion: if ncm-ncd is called in noaction mode would it still create the directory. I think it should create the directory regardless.

stdweird commented 6 years ago

@ttyS4 it will still do it but i'll check (and even if it didn't you can pass keeps_state => 1 option to ignore noaction for that specific call

stdweird commented 6 years ago

as expected it is created (the noaction is only set when handling components, th etempdir is created before that

[DEBUG] directory: (tempdir) noaction is false
[VERB] Created temporary directory with tempdir: /tmp/ncm-ncd-XJ9HrT
[DEBUG] Created temp directory /tmp/ncm-ncd-XJ9HrT

ttyS4 commented 6 years ago

Sorry for making you wait long on this.

Other than making it fail if we can't enter tmpdir this looks good to me. My quick testing shows no errors.

Restoring chdir after each module exec seems unnecessary for me as we chdir before each module execution still probably does not hurt.

stdweird commented 6 years ago

@ttyS4 addressed your remarks, unittests pass (but not yet tested)

ttyS4 commented 6 years ago

Another quick check (without chroot) shows no errors.

jrha commented 6 years ago

@stdweird which PR does this replace? Right now it references itself...

ttyS4 commented 6 years ago

121

On January 15, 2018 5:18:35 PM GMT+01:00, James Adams notifications@github.com wrote:

@stdweird which PR does this replace? Right now it references itself...

-- Csillag Tamás mobilról

jrha commented 6 years ago

Thanks!

stdweird commented 6 years ago

@jrha @ttyS4 sorry, yes this replaces #121

jrha commented 6 years ago

@ttyS4 my concern about including this in the 17.12 release is down to the combination of how late the release is and how little I understand the potential impacts of this change.

stdweird commented 6 years ago

@jrha and it didn't spew anything in current master version?

jrha commented 6 years ago

Probably did, not complaining.

jrha commented 6 years ago

I've tested this enough to make myself happier, so in it goes...