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

[v8.1] [diracx] Get token and run integration tests #7143

Merged
merged 8 commits into from
Sep 5, 2023

Conversation

chaen
Copy link
Contributor

@chaen chaen commented Aug 1, 2023

This should be transparent :-)
It starts integrating diracx by:

  • enabling/disabling feature with env variables
  • allow it to run in the integration_tests

One thing could be disruptive though:(@fstagni ) The CSApi does not look anymore in Default or <Setup> for the shifter.
What's your plan for that migration ? Tell the people to copy everything under /Default to the root of the Operatons section before upgrading ?

BEGINRELEASENOTES
*CS
CHANGE: do not look for the shifter under the Default or section

*Test
NEW: allow integration tests to run against DiracX

*Core

NEW: get a token for a proxy

*WMS
NEW: call the diracx Job Monitoring endpoint

ENDRELEASENOTES

@chaen chaen closed this Sep 1, 2023
@chaen chaen reopened this Sep 1, 2023
@chaen chaen marked this pull request as ready for review September 1, 2023 15:32
chrisburr
chrisburr previously approved these changes Sep 3, 2023
@fstagni
Copy link
Contributor

fstagni commented Sep 4, 2023

On this:

One thing could be disruptive though:(@fstagni ) The CSApi does not look anymore in Default or <Setup> for the shifter. What's your plan for that migration ? Tell the people to copy everything under /Default to the root of the Operatons section before upgrading ?

First of all, why would you need to change that?

We said to remove the "Setup" concept, and several steps have been done in this direction (see notes for https://github.com/DIRACGrid/DIRAC/wiki/DIRAC-8.1). But, the changes up to now have been done only at the database level, and no changes in CS have been planned. "Setup" is everywhere in CS and no detailed plan have been drawn, and really the latest updates were in #5287. For this release we were not planning CS changes as the complexity of upgrading to v8.1 is already significant, but if there's a really good reason...

tests/CI/docker-compose.yml Show resolved Hide resolved
integration_tests.py Show resolved Hide resolved
integration_tests.py Outdated Show resolved Hide resolved
src/DIRAC/FrameworkSystem/scripts/dirac_proxy_init.py Outdated Show resolved Hide resolved
# pylint: disable=import-error
import os

if os.getenv("DIRAC_ENABLE_DIRACX_JOB_MONITORING", "No").lower() in ("yes", "true"):
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition is already applied in src/DIRAC/WorkloadManagementSystem/Client/JobMonitoringClient.py.

In general, I think that we can put all this code in src/DIRAC/WorkloadManagementSystem/Client/JobMonitoringClient.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed because otherwise sphinx crashes. diracx is not a dependency of DIRAC so we have no other choice that hide all the diracx code behind env variables

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you add diracx to

DIRAC_DOC_MOCK_LIST = [
?

How does sphinx crash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try thanks !

tests/CI/check_db_initialized.sh Outdated Show resolved Hide resolved
tests/CI/exportCSLoop.sh Show resolved Hide resolved
@chaen
Copy link
Contributor Author

chaen commented Sep 4, 2023

On this:

One thing could be disruptive though:(@fstagni ) The CSApi does not look anymore in Default or <Setup> for the shifter. What's your plan for that migration ? Tell the people to copy everything under /Default to the root of the Operatons section before upgrading ?

First of all, why would you need to change that?

When running the export script, it crashes without that, as there are no setup known

We said to remove the "Setup" concept, and several steps have been done in this direction (see notes for https://github.com/DIRACGrid/DIRAC/wiki/DIRAC-8.1). But, the changes up to now have been done only at the database level, and no changes in CS have been planned. "Setup" is everywhere in CS and no detailed plan have been drawn, and really the latest updates were in #5287. For this release we were not planning CS changes as the complexity of upgrading to v8.1 is already significant, but if there's a really good reason...

I think that saying that before upgrading to v8.1 it is necessary to copy all the /Operations/<setup/default> to /Operations/<vo> or /Operations is not too much of a change.

@fstagni
Copy link
Contributor

fstagni commented Sep 4, 2023

I think that saying that before upgrading to v8.1 it is necessary to copy all the /Operations/<setup/default> to /Operations/<vo> or /Operations is not too much of a change.

That's a bit more than than. Something like

Operations
{
	Defaults
	{
		someOption = A
	}
	someSetup
	{
		someOption = B
	}
	vo
	{
		Defaults
		{
			someOption = C
		}
		someSetup
		{
			someOption = D
		}
	}
}

Should become

Operations
{
	someOption = B
	vo
	{
		someOption = D
	}
}

or even

Operations
{
	vo
	{
		someOption = D
	}
}

all this supposing that someSetup actually exists. So, depending on how much crap is these sections, it might become a lot of manual work. If you make a script though that handle all these cases...

@chrisburr
Copy link
Member

We think we still need Default for multi-vo so it can be layered on top of each VO's config without duplicating everything needlessly.

@chaen
Copy link
Contributor Author

chaen commented Sep 4, 2023

We think we still need Default for multi-vo so it can be layered on top of each VO's config without duplicating everything needlessly.

I am not sure I understand.

What you suggests is the following ?

Operations
{
        Default
        {
	      someOption = B
        }
	vo
	{
		someOption = D
	}
}

@chrisburr chrisburr self-requested a review September 5, 2023 07:24
@chrisburr chrisburr dismissed their stale review September 5, 2023 07:24

CS changes for multi-VO

@fstagni
Copy link
Contributor

fstagni commented Sep 5, 2023

We think we still need Default for multi-vo so it can be layered on top of each VO's config without duplicating everything needlessly.

I am not sure I understand.

What you suggests is the following ?

Operations
{
        Default
        {
	      someOption = B
        }
	vo
	{
		someOption = D
	}
}

The above is just to say that what is below is a case:

Operations
{
        Default
        {
	      someOption = B
              other_Option = BBB
              yet_another_Option = BBBBBBB
        }
	vo
	{
		someOption = D
	}
	another_vo
	{
		other_Option = AAA
	}
}

@chaen
Copy link
Contributor Author

chaen commented Sep 5, 2023

I am now very confused. What I propose is that what is contained in the Setup or Default is promoted one level higher.

Operations
{
	Defaults
	{
		someOptionInGlobolDefault = A
	}
	someSetup
	{
		someOptionInGlobalSetup = B
	}
	vo
	{
		Defaults
		{
			someOptionInVoDefault = C
		}
		someSetup
		{
			someOptionInVoSetup = D
		}
	}
}

becomes

Operations
{
        # Remove default and Setup
	someOptionInGlobolDefault = A
	someOptionInGlobalSetup = B
	vo
	{
               # Remove default and Setup
		someOptionInVoDefault = C
		someOptionInVoSetup = D
	}
}

What is the use case not satisfied with this ?

@fstagni
Copy link
Contributor

fstagni commented Sep 5, 2023

If the absence of "Defaults" is interpreted like today's "Defaults" then I think it's also fine. Operations helper need to of course be updated in v8.0 for that.

@chaen
Copy link
Contributor Author

chaen commented Sep 5, 2023

I think it even eases the migration because everything can just be copied one level up, prior to migrating to v8.1, so we don't even need to touch the code of v8.0
And then we remove the older section

@fstagni fstagni merged commit 262eab1 into DIRACGrid:integration Sep 5, 2023
24 checks passed
@DIRACGridBot DIRACGridBot added the sweep:ignore Prevent sweeping from being ran for this PR label Sep 5, 2023
@chaen chaen deleted the getToken_integration branch June 11, 2024 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sweep:ignore Prevent sweeping from being ran for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants