Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore[KFLUXINFRA-866] Add default tags to MPC VM(s) #306

Merged
merged 4 commits into from
Oct 7, 2024

Conversation

mshaposhnik
Copy link
Collaborator

@mshaposhnik mshaposhnik commented Oct 1, 2024

Ideally, all konflux resources must be appropriately tagged.
IBM doesnt seem to allow to add custom tags to VMs.
Fixes https://issues.redhat.com/browse/KFLUXINFRA-866

@ifireball
Copy link
Member

What is our motivation for doing this? How are we planning to process this tag information?

@mshaposhnik
Copy link
Collaborator Author

mshaposhnik commented Oct 1, 2024

@ifireball There is epic exists that ALL konflux resourves should be specifically tagged (https://issues.redhat.com/browse/KFLUXINFRA-720), and this PR is just a MPC part of it.
We have nothing specific to do with them.

@mshaposhnik mshaposhnik requested a review from ifireball October 1, 2024 13:56
pkg/aws/ec2.go Outdated Show resolved Hide resolved
@mshaposhnik mshaposhnik requested a review from hugares October 4, 2024 10:09
@hugares
Copy link
Contributor

hugares commented Oct 4, 2024

Do we have an existing documentation for the MPC configuration, I mean in this repository?

@ifireball
Copy link
Member

@ifireball There is epic exists that ALL konflux resourves should be specifically tagged (https://issues.redhat.com/browse/KFLUXINFRA-720), and this PR is just a MPC part of it. We have nothing specific to do with them.

I couldn't find the answer so I ask again here, why do we want this? Also someone looking in to the Git history of this project shouldn't have to dig into Jira to find the reasons behind changes being made. "Ideally, all konflux resources must be appropriately tagged." is not an answer - why is that ideal?

@mshaposhnik
Copy link
Collaborator Author

@eisraeli could you please provide a more motivation for this? b/c i just see what is written in the epic and it is seems not enough for eveyone

@eisraeli
Copy link

eisraeli commented Oct 7, 2024

@eisraeli could you please provide a more motivation for this? b/c i just see what is written in the epic and it is seems not enough for eveyone

Hey @mshaposhnik
The tagging is part of an epic that Manish created:
https://issues.redhat.com/browse/KFLUXINFRA-720

@manish-jangra Was this specific set of tags a requirement by IT or Cloudability ? I can't find any details on the epic.

Copy link

@manish-jangra manish-jangra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@manish-jangra
Copy link

manish-jangra commented Oct 7, 2024

@ifireball There is epic exists that ALL konflux resourves should be specifically tagged (https://issues.redhat.com/browse/KFLUXINFRA-720), and this PR is just a MPC part of it. We have nothing specific to do with them.

I couldn't find the answer so I ask again here, why do we want this? Also someone looking in to the Git history of this project shouldn't have to dig into Jira to find the reasons behind changes being made. "Ideally, all konflux resources must be appropriately tagged." is not an answer - why is that ideal?

@ifireball These tags will help us fetch reports from cloudability using those tags. Eyal has been asking to tag them as per the IT policy.

@ifireball
Copy link
Member

@ifireball There is epic exists that ALL konflux resourves should be specifically tagged (https://issues.redhat.com/browse/KFLUXINFRA-720), and this PR is just a MPC part of it. We have nothing specific to do with them.

I couldn't find the answer so I ask again here, why do we want this? Also someone looking in to the Git history of this project shouldn't have to dig into Jira to find the reasons behind changes being made. "Ideally, all konflux resources must be appropriately tagged." is not an answer - why is that ideal?

@ifireball These tags will help us fetch reports from cloudability using those tags. Eyal has been asking to tag them as per the IT policy.

Ok, so its a very Red Hah specific think - which means we probably don't want to hard-code the values in the MPC codebase

@ifireball
Copy link
Member

@mshaposhnik just to make sure I understand - all this change is doing ATM as adding an "extra tags" configuration field that causes more tags to be added to the VM?

I remember seeing some specific values at some point, was that removed?

@mshaposhnik
Copy link
Collaborator Author

@ifireball yes. it turns that those tags must be optional and little different bertween clusters, so i moved them into conf.
redhat-appstudio/infra-deployments#4656 is the configuration PR.

@ifireball
Copy link
Member

@mshaposhnik

@ifireball yes. it turns that those tags must be optional and little different bertween clusters, so i moved them into conf. redhat-appstudio/infra-deployments#4656 is the configuration PR.

oh, I kinda missed addional-instance-tags being a global option that applies to all VM kinds - we may want to make it a per-pool sub-option, but that would mean a lot of repetition in the config file - so we might want to enable some form of configuration inheritance. Then again I'm not inclined to add complexity we don't really need.

I wonder about the app-code, Project, Owner and cost-center fields we end up adding to the tag, shouldn't we try to make those reflect the team running the pipeline rather then be generic Cluster-wide values?

@mshaposhnik
Copy link
Collaborator Author

mshaposhnik commented Oct 7, 2024

I wonder about the app-code, Project, Owner and cost-center fields we end up adding to the tag, shouldn't we try to make those reflect the team running the pipeline rather then be generic Cluster-wide values?

@ifireball I am a wrong person to ask this kind of questions, it's more for @eedri or @manish-jangra.
In my personal opinion - no, as we're running those instances on behalf of our service, not the end users.

@ifireball
Copy link
Member

I wonder about the app-code, Project, Owner and cost-center fields we end up adding to the tag, shouldn't we try to make those reflect the team running the pipeline rather then be generic Cluster-wide values?

@ifireball I am a wrong person to ask this kind of questions, it's more for @eedri or @manish-jangra. In my personal opinion - no, as we're running those instances on behalf of our service, not the end users.

I'd say the entire service including but not limited to the BM instances, runs on behalf of the users. But now that you mention it, things can get very complicated when you factor in multi-use VMs and pools. Ok. So I'll parove this as-is for now, with the understanding that we'll probably need to layer more stuff on top of it sooner or later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants