openshift_secondary_metadata_parser: Add 'version' parsing #314

wking · 2020-10-05T17:54:51Z

Consuming the version file that graph-data grew in openshift/cincinnati-graph-data#233, so the logs will contain more useful error messages when the plugin is fed an incompatible schema version.

vrutkovs · 2020-10-05T18:16:30Z

cincinnati/src/plugins/internal/graph_builder/openshift_secondary_metadata_parser/plugin.rs

@@ -295,6 +295,26 @@ impl OpenshiftSecondaryMetadataParserPlugin {
        }
    }

+    fn process_version(


Lets declare this pub (same for supported_version) so that it could be reused in porting cincinnati-graph-data to Rust

Wouldn't we just run this whole plugin in the one-shot CI tool?

yeah, but it needs to be public so that we could import it in cincinnati-graph-data CI too

@vrutkovs In the case which @wking describes we wouldn't need to use any code directly, but we'd run an appropriately configured graph-builder instead.

so, eventually, in cincinnati-graph-data we'd run one-shot graph-builder? Sounds good to me, its a long shot though

Is a one-shot invocation of this plugin more achievable?

From the plugin perspective there's no other mode as one-shot as they don't compile to standalone binaries. It needs an embedding binary which in our case could be the graph-builder or the policy-engine. Just to mention it, I've toyed with the idea to make all of these the same binary with several sub-commands.

openshift-ci-robot · 2020-10-05T18:18:47Z

@wking: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/rustfmt-1.46.0	cf1c808927d16d30f39c70d1c69e74f92f18cbf9	link	`/test rustfmt-1.46.0`
ci/prow/e2e	cf1c808927d16d30f39c70d1c69e74f92f18cbf9	link	`/test e2e`
ci/prow/images	cf1c808927d16d30f39c70d1c69e74f92f18cbf9	link	`/test images`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vrutkovs

A better idea for iterate and check loop:

        let str_version = String::from_utf8_lossy(&version);
        if supported_versions.contains(&str_version.trim()) {
            Ok(())
        } else {
            Err(format_err!(
                "unrecognized graph-data version {}; supported versions: {:?}",
                str_version,
                supported_versions
            ))
        }

cincinnati/src/plugins/internal/graph_builder/openshift_secondary_metadata_parser/plugin.rs

vrutkovs

looks good so far, one nit-pick

cincinnati/src/plugins/internal/graph_builder/openshift_secondary_metadata_parser/plugin.rs

wking · 2020-10-31T23:40:21Z

Green :). I've dropped the diff edits and moved to just the version handling and fixture bump with bd5d7e3f -> eaad2cbe.

wking · 2020-11-03T22:40:15Z

All green, and I think I've addressed all the earlier review feedback or dropped the changes being reviewed ;). One remaining thread is "public or not?", and I'd rather punt on that for now, and see if we can get the one-shot plugin approach going for graph-data CI.

/assign @steveej @vrutkovs

steveej · 2020-11-04T09:57:02Z

I'd find it cleaner if we split the version bump at least into a separate commit, and preferably accomplish it by merging the separate #343 first.

Node order has no semantic significance, but folks reading Cincy JSON by eye may have an easier time of things if the nodes are sorted by SemVer version and key order is stable within each node. This script will ingest arbitrary Cincinnati JSON on stdin and write the stable, ordered analog to stdout. Using this on our test fixtures will also make fixture diffs slightly more human readable.

Generated with: $ for X in e2e/tests/testdata/*.json; do A="$(hack/graph-normalize.sh < "${X}")"; echo "${A}" > "${X}"; done

In preparation for consuming the 'version' file that graph-data grew in [1]. This gets us the first main-line commit to include the 'version' file: cincinnati-graph-data$ git --no-pager show --date=short --format='%ad %h %s' d980578d2e b87e7c2782 2020-03-18 d980578 Merge pull request openshift#125 from marun/unique-service-ca-serial 2020-05-11 b87e7c2 Merge pull request openshift#233 from wking/version-file [1]: openshift/cincinnati-graph-data#233

Dropping the production suffix. I don't really care where the fixture data comes from, but get-graph-pe-staging and get-graph-pe-production will only be serving a single graph-data commit at once. Using get-graph-pe instead will make it easier for folks to use this Justfile to get graph dumps based on arbitrary graph-data commits which they have fed into their local graph builder to be consumed by their local policy engine. With this change, generating new fixtures should work like: $ echo "${GRAPH_DATA_COMMIT_HASH}" >e2e/tests/testdata/metadata_revision $ just run-graph-builder-e2e & $ just just run-policy-engine & $ just e2e-fixtures-capture-only $ killall just Also rename our current fixtures to match the new, suffix-less pattern, and update the e2e suite to no longer require the suffix.

Consuming the 'version' file that graph-data grew in [1], so the logs will contain more useful error messages when the plugin is fed an incompatible schema version. Bumping the graph-data commit used for CI moves us to the first main-line commit to include the 'version' file: cincinnati-graph-data$ git --no-pager show --date=short --format='%ad %h %s' d980578d2e b87e7c2782 2020-03-18 d980578 Merge pull request openshift#125 from marun/unique-service-ca-serial 2020-05-11 b87e7c2 Merge pull request openshift#233 from wking/version-file [1]: openshift/cincinnati-graph-data#233

wking · 2020-11-05T19:32:42Z

Rebased on top of #343 with eaad2cbe -> 60a9aca.

steveej · 2020-11-05T19:33:38Z

@wking do you want to merge this as is then or wait until and rebase again when #343 is merged?

steveej

/lgtm

openshift-ci-robot · 2020-11-05T19:36:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: steveeJ, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [steveeJ,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

wking · 2021-03-19T04:26:01Z

/cherrypick release-4.6

openshift-cherrypick-robot · 2021-03-19T04:26:41Z

@wking: new pull request created: #379

In response to this:

/cherrypick release-4.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

So we can explain why we're blocking the different edges [1] (the promql -> PromQL type change is in flight with [2]). The zz in the filename for the vSphere hostname block ensures that one sorts last alphabetically, because it's the broadest block, and legacy Cincinnati services will prefer the final regular expression they load for a given 'to' target. This is basically a second attempt at my earlier 39bc2fb (blocked-edges/4.7.4*: Targeted edge blocking and version 1.1.0, 2021-09-01, openshift#1056), which ended up getting reverted in da1254a (Revert "blocked-edges/4.7.4*: Targeted edge blocking and version 1.1.0", 2021-09-21, openshift#1078) because the production Cincinnati was mad about the 1.1.0 version string. [3] taught Cincinnati to relax, and now that's live (and we never shipped any versions that would be mad about 1.1.0 to customers, the 4.6.0 Update Service operator went out before [4]). [1]: openshift/enhancements#821 [2]: openshift/enhancements#910 [3]: openshift/cincinnati#538 [4]: openshift/cincinnati#314

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 5, 2020

openshift-ci-robot requested review from jottofar and LalatenduMohanty October 5, 2020 17:55

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2020

vrutkovs reviewed Oct 5, 2020

View reviewed changes

vrutkovs requested changes Oct 6, 2020

View reviewed changes

LalatenduMohanty requested changes Oct 6, 2020

View reviewed changes

cincinnati/src/plugins/internal/graph_builder/openshift_secondary_metadata_parser/plugin.rs Outdated Show resolved Hide resolved

wking force-pushed the graph-data-version branch 6 times, most recently from 970b318 to ce738f5 Compare October 27, 2020 22:04

wking changed the title ~~WIP: openshift_secondary_metadata_parser: Add 'version' parsing~~ openshift_secondary_metadata_parser: Add 'version' parsing Oct 27, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 27, 2020

wking force-pushed the graph-data-version branch 8 times, most recently from d229e3c to 372989d Compare October 29, 2020 05:51

vrutkovs reviewed Oct 29, 2020

View reviewed changes

cincinnati/src/plugins/internal/graph_builder/openshift_secondary_metadata_parser/plugin.rs Outdated Show resolved Hide resolved

wking force-pushed the graph-data-version branch 3 times, most recently from af88b44 to 3b3e674 Compare October 29, 2020 18:07

vrutkovs reviewed Oct 29, 2020

View reviewed changes

cincinnati/src/plugins/internal/graph_builder/openshift_secondary_metadata_parser/plugin.rs Outdated Show resolved Hide resolved

wking force-pushed the graph-data-version branch from 3b3e674 to 89c15ec Compare October 29, 2020 20:25

wking force-pushed the graph-data-version branch 4 times, most recently from bd5d7e3 to eaad2cb Compare October 31, 2020 23:40

openshift-ci-robot assigned steveej and vrutkovs Nov 3, 2020

wking added 5 commits November 5, 2020 11:04

e2e/tests/testdata: Normalize Cincinnati JSON

6515e3b

Generated with: $ for X in e2e/tests/testdata/*.json; do A="$(hack/graph-normalize.sh < "${X}")"; echo "${A}" > "${X}"; done

wking force-pushed the graph-data-version branch from eaad2cb to 60a9aca Compare November 5, 2020 19:32

steveej approved these changes Nov 5, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2020

openshift-merge-robot merged commit 528bf54 into openshift:master Nov 5, 2020

LalatenduMohanty mentioned this pull request Jan 22, 2021

We do not need patch manager approval for cherry picks for Cincinnati openshift/release#15179

Merged

wking deleted the graph-data-version branch March 19, 2021 04:25

openshift-cherrypick-robot mentioned this pull request Mar 19, 2021

Bug 1940747: openshift_secondary_metadata_parser: Add 'version' parsing #379

Closed

wking mentioned this pull request Sep 23, 2021

blocked-edges/4.7.4*: Targeted edge blocking and version 1.1.0 openshift/cincinnati-graph-data#1089

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openshift_secondary_metadata_parser: Add 'version' parsing #314

openshift_secondary_metadata_parser: Add 'version' parsing #314

wking commented Oct 5, 2020

vrutkovs Oct 5, 2020

wking Oct 6, 2020

vrutkovs Oct 6, 2020

steveej Oct 6, 2020

vrutkovs Oct 6, 2020

wking Oct 7, 2020

steveej Oct 8, 2020

openshift-ci-robot commented Oct 5, 2020

vrutkovs left a comment

vrutkovs left a comment

wking commented Oct 31, 2020

wking commented Nov 3, 2020

steveej commented Nov 4, 2020 •

edited

Loading

wking commented Nov 5, 2020

steveej commented Nov 5, 2020

steveej left a comment

openshift-ci-robot commented Nov 5, 2020

wking commented Mar 19, 2021

openshift-cherrypick-robot commented Mar 19, 2021

openshift_secondary_metadata_parser: Add 'version' parsing #314

openshift_secondary_metadata_parser: Add 'version' parsing #314

Conversation

wking commented Oct 5, 2020

vrutkovs Oct 5, 2020

Choose a reason for hiding this comment

wking Oct 6, 2020

Choose a reason for hiding this comment

vrutkovs Oct 6, 2020

Choose a reason for hiding this comment

steveej Oct 6, 2020

Choose a reason for hiding this comment

vrutkovs Oct 6, 2020

Choose a reason for hiding this comment

wking Oct 7, 2020

Choose a reason for hiding this comment

steveej Oct 8, 2020

Choose a reason for hiding this comment

openshift-ci-robot commented Oct 5, 2020

vrutkovs left a comment

Choose a reason for hiding this comment

vrutkovs left a comment

Choose a reason for hiding this comment

wking commented Oct 31, 2020

wking commented Nov 3, 2020

steveej commented Nov 4, 2020 • edited Loading

wking commented Nov 5, 2020

steveej commented Nov 5, 2020

steveej left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Nov 5, 2020

wking commented Mar 19, 2021

openshift-cherrypick-robot commented Mar 19, 2021

steveej commented Nov 4, 2020 •

edited

Loading