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

Handle partitioned and updating billing exports #2794

Conversation

Sean-Holcomb
Copy link
Contributor

What does this PR change?

This PR updates the Azure storage Cloud Cost to allow it to work with billing exports with various setting for partitioned and file overwrite. Until now only unpartitioned non-overwriting billing exports were supported.

To support file overwrite we need to make sure that we are redownloading a blob if there have been updates to it since the last time we downloaded it. This is a simple of process of comparing the mod time of the blob and the local version if it has already been downloaded.

To support partitioned exports we look for the presence of a manifest.json file which contains a list of the blobs that contain the partitions, if this is located instead of adding the newest file for a month we add all blobs named in the manifest.

Does this PR relate to any other PRs?

How will this PR impact users?

Does this PR address any GitHub or Zendesk issues?

  • Closes ...

How was this PR tested?

  • This PR was tested via integration test against configs with unpartitioned non-replacement and partitioned replacement exports, resulting in the correct blobs being selected and valid cloud cost set being generated

Does this PR require changes to documentation?

Have you labeled this PR and its corresponding Issue as "next release" if it should be part of the next OpenCost release? If not, why not?

// Container. It uses the "Last Modified Time" of the file to determine which
// has the latest month-to-date billing data.
func (asbp *AzureStorageBillingParser) getMostRecentBlobs(start, end time.Time, client *azblob.Client, ctx context.Context) ([]string, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pass entire blob info object up rather than just the name to preserve mod time info which is used later on

blobModTime := *blob.Properties.LastModified
// Check if the blob was last modified before the file was modified, indicating that the
// file is the most recent version of the blob
if blobModTime.Before(fileInfo.ModTime()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this is not true? In other words, if the Blob file has a more recent timestamp than our local file's timestamp? Does this mean we need to go fetch the updated CSV file from storage?

^^ I don't think that scenario would ever happen though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops nevermind. After re-reading this, I see that we would proceed to download the updated Blob file! This comment is good to close.

return nil, fmt.Errorf("failed to retrieve manifest %w", err)
}

log.Infof(string(manifestBytes))
Copy link
Contributor

@thomasvn thomasvn Jun 11, 2024

Choose a reason for hiding this comment

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

Suggested change
log.Infof(string(manifestBytes))

Was this used for debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, yeah that is what that is

@@ -47,27 +48,29 @@ func (asbp *AzureStorageBillingParser) ParseBillingData(start, end time.Time, re
}
ctx := context.Background()
// Example blobNames: [ export/myExport/20240101-20240131/myExport_758a42af-0731-4edb-b498-1e523bb40f12.csv ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Example blobNames: [ export/myExport/20240101-20240131/myExport_758a42af-0731-4edb-b498-1e523bb40f12.csv ]

Comment here is probably no longer relevant?

@Sean-Holcomb Sean-Holcomb force-pushed the sth/partitioned-azure-billing-export branch from 26adf14 to 2700363 Compare June 18, 2024 19:08
@Sean-Holcomb Sean-Holcomb merged commit d866000 into opencost:develop Jun 18, 2024
3 checks passed
Sean-Holcomb added a commit to Sean-Holcomb/opencost that referenced this pull request Jun 18, 2024
@Sean-Holcomb Sean-Holcomb deleted the sth/partitioned-azure-billing-export branch June 18, 2024 19:19
Sean-Holcomb added a commit that referenced this pull request Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants