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

Schema: node attribute updates from multiple intel modules do not work #1210

Closed
achantavy opened this issue Jul 14, 2023 · 0 comments
Closed
Labels
schema Graph data schema issues

Comments

@achantavy
Copy link
Contributor

achantavy commented Jul 14, 2023

Description:
In #1154 I tried to refactor EBSVolumes to the new data model but ran into the following problem:

Expected behavior:
After refactoring the EBS Volume sync to the new model, I ran the EC2 instance sync and the EBS volume sync one after the other and see the deleteontermination field on the EBSVolumes that are attached to EC2 instances.

Actual behavior:
After refactoring, when I run the EC2 instance sync and the EBS volume sync one after the other, the deleteontermination field is not present.

Root cause:
The EC2 instance sync loads in :EC2Instances with :EBSVolumes attached based on the output of describe-ec2-instances. This data includes a field called deleteontermination on the :EBSVolume. deleteontermination is only set during the EC2 instance sync.

Next when the EBS Volume sync runs, it enriches existing :EBSVolumes with additional fields from data retrieved with describe-ebs-volumes but because this API call does not include deleteontermination, we will overwrite the previous values of deleteontermination with None, thus removing the property from all :EBSVolumes.

#1154 tries to solve this by updating the PropertyRef implementation to use COALESCE() so that any existing values already defined for a given node property are preserved. This is very quirky and complicated and probably going to introduce lots of new problems. Instead, I'd like to explore doing this with separate classes for properties derived from different API responses.

This needs to be solved in order for the new data model to work.

@achantavy achantavy added the schema Graph data schema issues label Jul 14, 2023
achantavy added a commit that referenced this issue Jul 14, 2023
achantavy added a commit that referenced this issue Jul 14, 2023
…iple intel modules (#1214)

See #1210 for full context.

#1154 tried to solve this problem by updating the querybuilder but this
was too complex and would not generalize well.

This solution is simpler where we use different property classes for
each API response so that we don't overwrite properties on a node set by
another sync job.

This PR can be reviewed commit-by-commit:
- c0d9ac4 shows a repro of the error
with a failing integration test.
- facb63b shows the solution using
multiple classes.

---------

Co-authored-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
ramonpetgrave64 added a commit that referenced this issue Aug 3, 2023
…iple intel modules (#1214)

See #1210 for full context.

#1154 tried to solve this problem by updating the querybuilder but this
was too complex and would not generalize well.

This solution is simpler where we use different property classes for
each API response so that we don't overwrite properties on a node set by
another sync job.

This PR can be reviewed commit-by-commit:
- c0d9ac4 shows a repro of the error
with a failing integration test.
- facb63b shows the solution using
multiple classes.

---------

Co-authored-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
chandanchowdhury pushed a commit to juju4/cartography that referenced this issue Jun 26, 2024
…pdates from multiple intel modules (cartography-cncf#1214)

See cartography-cncf#1210 for full context.

cartography-cncf#1154 tried to solve this problem by updating the querybuilder but this
was too complex and would not generalize well.

This solution is simpler where we use different property classes for
each API response so that we don't overwrite properties on a node set by
another sync job.

This PR can be reviewed commit-by-commit:
- c0d9ac4 shows a repro of the error
with a failing integration test.
- facb63b shows the solution using
multiple classes.

---------

Co-authored-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
schema Graph data schema issues
Projects
None yet
Development

No branches or pull requests

1 participant