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

Runcmsgridpatch req #3789

Merged
merged 11 commits into from
Oct 29, 2024
Merged

Runcmsgridpatch req #3789

merged 11 commits into from
Oct 29, 2024

Conversation

efeyazgan
Copy link
Collaborator

No description provided.

@lviliani
Copy link
Contributor

lviliani commented Oct 29, 2024

The test is failing because of an error in copying the patched gridpacks to eos. Could it be that eos is not mounted within the github CI?
@efeyazgan confirmed that local tests are fine and that similar patches worked in the past.
Could it be related to some of the recent updates by @ggonzr?
@DickyChant @sihyunjeon

@DickyChant
Copy link
Contributor

The test is failing because of an error in copying the patched gridpacks to eos. Could it be that eos is not mounted within the github CI? @efeyazgan confirmed that local tests are fine and that similar patches worked in the past. Could it be related to some of the recent updates by @ggonzr? @DickyChant @sihyunjeon

To copy a gridpack to eos, we need certain permissions.

The CI works as a runner on a self-hosted openstack VM, the linux user is "runner" and I definitely do not think it can copy anything to the GEN eos dir.

@lviliani
Copy link
Contributor

Thanks @DickyChant, it makes sense indeed.
So if everything looks good we can proceed and test it in production.

@DickyChant
Copy link
Contributor

To make it clear:

The CI is introduced by @mseidel42 in 2022 at this commit: 5784e58 while the similar previous fix should be introduced around MT discussion, i.e., 2020 before I entered graduate school and joined CMS.

PdmV never touches the scripts itself, and ofc the CI running has nothing to do with PdmV infrastucture. That being said, @ggonzr we should not hesitate to lend a hand if GEN wants some reference on how to deploy CI jobs on lxplus via Jenkins ;)

If it runs locally fine at lxplus, it should also be running fine at production, since it is a matter of whether pdmvserv, the service account running every McM job, can access eos or not. I think @bbilin might know? I think we could offer a try there. For example, there is a set of TOP request that are awaiting injection. We could test on those. I will link this patch there

@efeyazgan
Copy link
Collaborator Author

Thanks for the clarification. Yes, until now we had no problem accessing eos in McM jobs. So, we can bypass this test (but let me do a few final checks firtst).

In fact, I think I can add something to check if the user is "runner", then it can bypass checks that includes access to eos.

@lviliani @bbilin in any case, after all gridpacks are patched, the script will bypass this one as well.

@mseidel42
Copy link
Collaborator

Hi all, I can confirm that there is no EOS access from the runner, and it is cumbersome to set up with a service account.
If there is a Jenkins-based solution by PdmV already then it might be good to switch to that at some point.

In fact, I think I can add something to check if the user is "runner", then it can bypass checks that includes access to eos.

That sounds very fragile! Better to add another option like this and call the CI with --apply_run3_patch=false

parser.add_argument('--apply_run3_patch', help="add description what this actually does", action='store_true')

@Cvico
Copy link
Contributor

Cvico commented Oct 29, 2024

Hi,

As mentioned somewhere else in the thread, we have a set of requests that we want to inject, and they would need this patch applied. Is there anyway TOP can help to validate this, and also benefit from the patch?

@efeyazgan
Copy link
Collaborator Author

Hi @Cvico I think we will have the new version of the script today. After that, I think the TOP requests could be injected and that would be the validation (at least for the request checking script making the patches.). It would be good to have the list of those requests, so that I can take a look as well. thanks.

@Cvico
Copy link
Contributor

Cvico commented Oct 29, 2024

Requests are listed in these tickets. So:
- 2016APV: TOP-2024Oct30-00001
- 2016: TOP-2024Oct30-00002
- 2017: TOP-2024Oct30-00003
- 2018: TOP-2024Oct30-00004

There are quite a lot of requests in this case.

@efeyazgan efeyazgan merged commit 9020191 into master Oct 29, 2024
6 checks passed
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