Make the datastream reindexing APIs multi-project aware

[joegallo]

Updates POST /_migration/reindex and related APIs to operate on a project state (or project metadata) throughout.

Most of this is pretty mechanical, the only part that was a bit creative was tracking the ProjectId in the ReindexDataStreamTask (so make sure that the approach I took there seems sane).

[masseyke]

the only part that was a bit creative was tracking the ProjectId in the ReindexDataStreamTask (so make sure that the approach I took there seems sane).

It looks sane to me, assuming that we get a distinct NodeConstruction for each project, and each NodeClient is tied to a specific project (I assume we do, but I don't know a lot about multiproject).

[masseyke]

It looks sane to me, assuming that we get a distinct NodeConstruction for each project, and each NodeClient is tied to a specific project (I assume we do, but I don't know a lot about multiproject).

On second thought, I'm not so sure. It sounds like the projectResolver returns whatever the project id for the current request is, and maybe at node construction time it's just the default project? In that case, we probably need to write the project id to the task params. Or change the persistent task framework to pass in a projectResolver to createTask.

[nielsbauman]

1. There is no need to convert to ProjectState here (we could call clusterService.state().metadata().getProject(projectId)).
2. I see we have a null-check below for persistentTask. While we should handle project deletions (even soft-deletes) gracefully and stop persistent tasks first, I think it doesn't hurt to have some defense against deleted projects here. We could do something like:

   var project = clusterService.state().metadata().projects().get(projectId);
   PersistentTasksCustomMetadata.PersistentTask<?> persistentTask = project == null ? null : PersistentTasksCustomMetadata.getTaskWithId(project, getPersistentTask());

   But other variations of that are fine too of course.

The same goes for similar changes in this and other files, and for obtaining sourceIndex in ReindexDataStreamIndexTransportAction.java. Generally, if a block of code assumes that something exists (e.g. a task/custom/index), I don't do an extra null-check for the project, but if the code defends against missing objects, I check for missing project to maintain the level of defense.

[joegallo]

14ac833 addresses your first point, but I'll have to grind a bit on the second one. (Which I'll do tomorrow.)

[joegallo]

Okay, I think I've handled your second point via ef53eb2.

[nielsbauman]

@masseyke before the persistent task framework starts a (project-scoped) persistent task, it will put the project ID in the thread context:

elasticsearch/server/src/main/java/org/elasticsearch/persistent/PersistentTasksNodeService.java

Lines 195 to 203 in 528bd9c

	if (projectId != null) {
	@FixForMultiProject(
	description = "Replace with ProjectResolver#executeOnProject once "
	+ "DefaultProjectResolver can ensure the header in threadContext"
	)
	final String projectIdString = projectId.id();
	threadPool.getThreadContext().putHeader(Task.X_ELASTIC_PROJECT_ID_HTTP_HEADER, projectIdString);
	}
	doStartTask(taskInProgress, executor, request);

The ProjectResolver in ReindexDataStreamPersistentTaskExecutor then picks up that project ID from the thread context.
Does that answer your concern, or were you talking about something else?

[masseyke]

The ProjectResolver in ReindexDataStreamPersistentTaskExecutor then picks up that project ID from the thread context.
Does that answer your concern, or were you talking about something else?

OK that makes sense. Looks good then!

[joegallo]

@nielsbauman for the scope of this kind of work does it make sense for me to be ignoring implied compilation changes that should happen in the tests, too? For example, there are calls to Metadata#getProject() in CopyLifecycleIndexMetadataTransportActionIT but I just ignored files like that.

[elasticsearchmachine]

Pinging @elastic/es-data-management (Team:Data Management)

[nielsbauman]

Thanks for addressing my comments. I added one more comment with one suggestion and one optional suggestion. After that we should be good to go.

[nielsbauman]

I have good news and I have bad news. The bad news is that I said something wrong before in #130035 (comment). I said

The same goes for similar changes in this and other files

which isn't actually true. In all the other cases in this PR, we use the project resolver to obtain a project metadata (or project state). That throws an exception if the project doesn't exist (so doing a null-check is redundant). The "good" news is that the null-checks aren't doing much harm either, so I'm also fine with leaving them in. Sorry about that.

The places where we do need to do a null-check are the line I'm commenting on here, the lines below in this file, and in the task executor in CopyLifecycleIndexMetadataTransportAction, because those places run async of requests and could thus (theoretically) run after a project has been deleted. You currently use getProject(projectId) in both places, but that will throw an exception if the project doesn't exist (I know, not super intuitive from the method name, maybe we should change that). We'll need to do projects().get(projectId) as I did in my previous suggestion.

[joegallo]

Okay, that makes sense. I reverted the extraneous null checking on the results of getProjectMetadata in 08c6012.

[nielsbauman]

LGTM, thanks a lot for the iterations!