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

Updating package registry snapshot distribution version #89776

Merged
merged 10 commits into from
Feb 4, 2021

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Jan 29, 2021

This PR updates the version of the snapshot package registry that's used by Fleet tests.

@ycombinator ycombinator marked this pull request as ready for review February 1, 2021 14:04
@ycombinator
Copy link
Contributor Author

cc: @mtojek @ruflin @ph

@nchaulet
Copy link
Member

nchaulet commented Feb 1, 2021

@ycombinator looks like some test need to be updated, I can do it if you want.

@ycombinator
Copy link
Contributor Author

looks like some test need to be updated, I can do it if you want.

@nchaulet that would be great, if you don't mind. Thanks!

@ycombinator
Copy link
Contributor Author

@nchaulet I just invited you to be a collaborator on my kibana fork, if you just want to push commits to this PR's branch there.

@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@nchaulet nchaulet added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0 labels Feb 1, 2021
@nchaulet
Copy link
Member

nchaulet commented Feb 1, 2021

Currently test are failing because we are trying to create an existing transform (related to #89802)

@skh What are you though on that should we ignore that error when we create an already existing transform?

@skh
Copy link
Contributor

skh commented Feb 2, 2021

@skh What are you though on that should we ignore that error when we create an already existing transform?

I agree to @kevinlog 's comment on #89802 , let's ignore it.

@ycombinator
Copy link
Contributor Author

I agree to @kevinlog 's comment on #89802 , let's ignore it.

Does this mean this PR can be merged as-is, or is there some more work needed to mark a test as ignored or something like that?

@nchaulet
Copy link
Member

nchaulet commented Feb 2, 2021

Does this mean this PR can be merged as-is, or is there some more work needed to mark a test as ignored or something like that?

There is some work to do to fix the issue, I will create a separate PR for that.

@ycombinator
Copy link
Contributor Author

There is some work to do to fix the issue, I will create a separate PR for that.

Thanks. If it makes sense for you to push commits to this PR directly, please feel free to do so. I sent you an invite to collaborate on my kibana fork.

Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

🚀

@ycombinator
Copy link
Contributor Author

Test failure looks relevant, @nchaulet?

             └- ✖ fail: Fleet Endpoints fleet_agent_flow should work
             │      Error: Agent should run the 'system' package
             │       at Assertion.assert (/dev/shm/workspace/parallel/1/kibana/packages/kbn-expect/expect.js:100:11)
             │       at Assertion.contain (/dev/shm/workspace/parallel/1/kibana/packages/kbn-expect/expect.js:447:10)
             │       at Context.<anonymous> (test/fleet_api_integration/apis/agents/complete_flow.ts:118:52)
             │       at Object.apply (/dev/shm/workspace/parallel/1/kibana/packages/kbn-test/src/functional_test_runner/lib/mocha/wrap_function.js:73:16)

@nchaulet
Copy link
Member

nchaulet commented Feb 2, 2021

Yes the error is relevant I missed a part during the install transform

[00:01:40]               │ proc [kibana]   log   [23:24:35.257] [error][fleet][plugins] [status_exception] Cannot start transform [endpoint.metadata_current-default-0.17.0] as it is already started. response from /_transform/endpoint.metadata_current-default-0.17.0/_start: {"error":{"root_cause":[{"type":"status_exception","reason":"Cannot start transform [endpoint.metadata_current-default-0.17.0] as it is already started."}],"type":"status_exception","reason":"Cannot start transform [endpoint.metadata_current-default-0.17.0] as it is already started."},"status":409}

body: transform.content,
});
} catch (err) {
// swallow the error if the transform already exists.
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, but I'd like to double-check with @kevinlog that this is safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

this looks okay with me - I will get @pzl to comment as well asap

Copy link
Member

Choose a reason for hiding this comment

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

putting the same version, yeah we should logically be able to assume (aside from local dev situations) that it contains the same content trying to be installed then. Looks good.

Generally, the transform is deleted before install, right? I wonder why that isn't happening here

Copy link
Member

Choose a reason for hiding this comment

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

The transform is deleted if the saved object contains the transform, if our saved object and the state of ES differ this could happen

await callCluster('transport.request', {
method: 'POST',
path: `/_transform/${transform.installationName}/_start`,
// Ignore error if the transform is already started
ignore: [409],
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, cc @kevinlog

Copy link
Contributor

Choose a reason for hiding this comment

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

same here, looks OK - will check with @pzl

Copy link
Member

Choose a reason for hiding this comment

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

as long as we don't think we're swallowing other problems that might be in a 409, sure

@ycombinator
Copy link
Contributor Author

@elasticmachine merge upstream

@skh
Copy link
Contributor

skh commented Feb 4, 2021

I think we can merge this once CI is happy -- @kevinlog any objections? I believe this should fix #89802 as well?

@kevinlog
Copy link
Contributor

kevinlog commented Feb 4, 2021

Looks OK to me - it seems we'd keep moving and not error when a transform of the same version is already installed. This seems OK to me as we'd always increment the version on the Endpoint package if there were new mappings, components, etc.

I'll get the final 👍 from @pzl

Copy link
Member

@pzl pzl left a comment

Choose a reason for hiding this comment

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

The only thing I would worry about is the ignore all 409's when starting

We might end up with a false confidence if other classes of errors come back with 409's. If we think a transform is running, and it is not.. then a lot of data will never populate.

That said, none of these changes are introducing anything worse, so it's 👍 , just want to point out that caution

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jen-huang
Copy link
Contributor

Thanks for this @ycombinator @nchaulet! We haven't updated the distribution in a while :)

@ycombinator ycombinator deleted the fleet-upd-pkg-reg-dist-version branch February 4, 2021 21:49
@ycombinator
Copy link
Contributor Author

@jen-huang Going forward, the distribution should be updated more frequently. I've added a manual step to the package registry release process (see last step in https://github.com/elastic/package-registry/blob/master/README.md#release).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants