feat(openstack-aws): Add owner/lifetime info in VM's metadata #207

kaleemsiddiqu · 2022-11-03T16:38:27Z

Owner and lifetime info will be added into VM's metadata for Openstack/AWS providers. This info will give info who owns the VM and what is the lifetime of the VM which will be used for notifying owners and cleaning up the resources based on lifetime of the VMs.
If owner info not fetched, user notified with custom error message for follow up action to take up.

Signed-off-by: ksiddiqu [email protected]

Tiboris · 2022-11-03T17:39:37Z

First of all thanks for the PR very much. Let us re-visit changes.

Just a note also a title of PR must comply with commit message check i have changed it a bit.
Fist note if possible could you divide this to commit per provider change?

src/mrack/transformers/transformer.py

kaleemsiddiqu · 2022-11-08T08:04:57Z

First of all thanks for the PR very much. Let us re-visit changes.

Just a note also a title of PR must comply with commit message check i have changed it a bit. Fist note if possible could you divide this to commit per provider change?

Updated code in two commits

src/mrack/transformers/transformer.py

pvoborni

Comments added, but in general:

I'd avoid excessive writes on "info" level. We could be more chatty on debug level, but still on the extend that it is usable for somebody who is investigating an issue.
methods should not only what their name says
unit tests needs fixing (I didn't look to deep in it, but probably global context is not initialized)
you might check with @Tiboris about the gssapi import - it being optional

src/mrack/transformers/transformer.py

src/mrack/providers/aws.py

Tiboris · 2022-11-10T09:09:44Z

@kaleemsiddiqu could you please also add line (with some comment)

require-owner = true

to the https://github.com/neoave/mrack/blob/main/src/mrack/data/mrack.conf ?

pvoborni

I've tested various scenarios and it works well.

So the only remaining part is the mrack.spec discussion. Otherwise it is good from my PoV.

Doc changes can be done in a separate MR.

Tiboris

Please add

Requires:       python3-gssapi

after line https://github.com/neoave/mrack/blob/main/mrack.spec#L46

Owner and lifetime info will be added into VM's metadata for Openstack provider. This info will give info who owns the VM and what is the lifetime of the VM which will be used for notifying owners and cleaning up the resources based on lifetime of the VMs. If owner info not fetched, user notified with custom error message for follow up action to take up. Signed-off-by: ksiddiqu <[email protected]>

Owner and lifetime info will be added into VM's metadata for AWS provider. This info will give info who owns the VM and what is the lifetime of the VM which will be used for notifying owners and cleaning up the resources based on lifetime of the VMs. If owner info not fetched, user notified with custom error message for follow up action to take up. Signed-off-by: ksiddiqu <[email protected]>

Integration test test_actions.py failing due to introduction of new option(require-owner) in mrack.conf as global_context was not set which is now being set using a fixture in test_actions.py Signed-off-by: ksiddiqu <[email protected]>

Tiboris · 2022-11-14T12:35:06Z

Thanks @kaleemsiddiqu for all the changes you did and updates. I would like to kindly ask you to change the spec file to add:

Recommends:       python3-gssapi

after line https://github.com/neoave/mrack/blob/main/mrack.spec#L46 which will create weak dependency on the package.

With addition to setup.py (https://github.com/neoave/mrack/blob/main/setup.py#L56 ) :

    extras_require={
        "krb-owner": ["gssapi"],
    },

gssapi pkg now required for new added mrack feature to add owner/lifetime info into the VM Signed-off-by: ksiddiqu <[email protected]>

Tiboris

Thanks for the patch @kaleemsiddiqu fixes and patience lets ship this ! :)

kaleemsiddiqu added the enhancement New feature or request label Nov 3, 2022

kaleemsiddiqu force-pushed the addition-of-owner-lifetime-mrack branch 2 times, most recently from 1e875ef to 4353266 Compare November 3, 2022 17:36

Tiboris changed the title ~~feat(openstack/aws): Add owner/lifetime info in VM's metadata~~ feat(openstack-aws): Add owner/lifetime info in VM's metadata Nov 3, 2022

Tiboris self-requested a review November 3, 2022 17:42

pvoborni reviewed Nov 4, 2022

View reviewed changes

src/mrack/transformers/transformer.py Outdated Show resolved Hide resolved

pvoborni requested changes Nov 4, 2022

View reviewed changes

kaleemsiddiqu force-pushed the addition-of-owner-lifetime-mrack branch 2 times, most recently from 500c9bb to 1718a4f Compare November 8, 2022 08:04

kaleemsiddiqu force-pushed the addition-of-owner-lifetime-mrack branch from 1718a4f to ae35a3f Compare November 8, 2022 08:58

Tiboris reviewed Nov 8, 2022

View reviewed changes

src/mrack/transformers/transformer.py Outdated Show resolved Hide resolved

pvoborni requested changes Nov 8, 2022

View reviewed changes

kaleemsiddiqu force-pushed the addition-of-owner-lifetime-mrack branch 5 times, most recently from bd1753d to 4aaa042 Compare November 8, 2022 19:31

Tiboris mentioned this pull request Nov 10, 2022

remove the python_disable_dependency_generator from mrack spec and properly divide python package #213

Open

kaleemsiddiqu force-pushed the addition-of-owner-lifetime-mrack branch from b2e9b63 to 91bde71 Compare November 10, 2022 08:46

pvoborni reviewed Nov 10, 2022

View reviewed changes

kaleemsiddiqu requested a review from pvoborni November 10, 2022 12:13

kaleemsiddiqu self-assigned this Nov 10, 2022

Tiboris requested changes Nov 11, 2022

View reviewed changes

kaleemsiddiqu added 3 commits November 11, 2022 18:55

fix: integration test_actions test fixes

4f681b5

Integration test test_actions.py failing due to introduction of new option(require-owner) in mrack.conf as global_context was not set which is now being set using a fixture in test_actions.py Signed-off-by: ksiddiqu <[email protected]>

kaleemsiddiqu force-pushed the addition-of-owner-lifetime-mrack branch from 91bde71 to 4f681b5 Compare November 11, 2022 13:26

kaleemsiddiqu requested a review from Tiboris November 11, 2022 13:40

pvoborni approved these changes Nov 14, 2022

View reviewed changes

chore: include optional dependency of gssapi

43a3b9e

gssapi pkg now required for new added mrack feature to add owner/lifetime info into the VM Signed-off-by: ksiddiqu <[email protected]>

Tiboris approved these changes Nov 14, 2022

View reviewed changes

Tiboris merged commit fd4e0db into neoave:main Nov 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(openstack-aws): Add owner/lifetime info in VM's metadata #207

feat(openstack-aws): Add owner/lifetime info in VM's metadata #207

kaleemsiddiqu commented Nov 3, 2022

Tiboris commented Nov 3, 2022 •

edited

Loading

kaleemsiddiqu commented Nov 8, 2022

pvoborni left a comment

Tiboris commented Nov 10, 2022 •

edited

Loading

pvoborni left a comment

Tiboris left a comment

Tiboris commented Nov 14, 2022 •

edited

Loading

Tiboris left a comment

feat(openstack-aws): Add owner/lifetime info in VM's metadata #207

feat(openstack-aws): Add owner/lifetime info in VM's metadata #207

Conversation

kaleemsiddiqu commented Nov 3, 2022

Tiboris commented Nov 3, 2022 • edited Loading

kaleemsiddiqu commented Nov 8, 2022

pvoborni left a comment

Choose a reason for hiding this comment

Tiboris commented Nov 10, 2022 • edited Loading

pvoborni left a comment

Choose a reason for hiding this comment

Tiboris left a comment

Choose a reason for hiding this comment

Tiboris commented Nov 14, 2022 • edited Loading

Tiboris left a comment

Choose a reason for hiding this comment

Tiboris commented Nov 3, 2022 •

edited

Loading

Tiboris commented Nov 10, 2022 •

edited

Loading

Tiboris commented Nov 14, 2022 •

edited

Loading