Add virtual repo snapshot feature to tdnf #497

sameluch · 2024-09-20T21:31:51Z

This PR adds some functionality built for Azure Linux to tdnf for the broader userbase, virtual repo snapshots. This is the ability to emulate an older snapshot of all enabled repos at a specific timestamp.

Added with this PR is a new config option snapshottime=<posix_time> which can be added to tdnf.conf to load as a global version of another added command line option --snapshottime=<posix_time>. The snapshot emulates as if the command was run against repos at the given timestamp. All packages which are published to a repo with a timestamp later than the the specified --snapshottime will be occluded from the solver and thus not considered for list, install, upgrade, and other commands which hit repositories.

Additionally, if some repos are desired to be not snapshot while executing a given path --snapshotexcluderepos=<repo_id1>[,<repo_id2>,...]. Each repo specified in --snapshotexcluderepos will NOT have the --snapshottime applied to it, if applicable.

vmwclabot · 2024-09-20T21:31:58Z

@sameluch, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

oliverkurth · 2024-09-20T22:09:22Z

Wow, that's a big change. It will take me a while to review this. I have a few question:

What is the use case? Is it just for testing, for example test an upgrade scenario? Or do you want to tag a specific time for a stable build?

Have you considered alternative approaches? For example, wouldn't it be easier to just create a "real" repository with all packages older than the snapshot time (if space is an issue, maybe just use symlinks?). I think this could be done using simple shell scripts, much simpler than this change.

How reliable is the "file" attribute, which is set (I guess) by createrepo? Does it use the actual file time stamp, or current time?

Have you thought about implementing this as a plugin? I know the plugin interface would probably need changes for this. But I think the use case is rather limited.

sshedi

I just did one round of review. Need to understand the logic and usage better.

Some generic comments:

Avoid sprintf, strcpy and prefer snprintf, strncpy
Use already available tdnf APIs for memory allocations
Make functions static if not used outside of file
Add test cases and try to cover as various scenarios

Please sign the SLA before anything else.

sshedi · 2024-09-26T05:43:45Z

client/repo.c

+    // take command line over config if both are present
+    for (pSetOpt = pTdnf->pArgs->pSetOpt; pSetOpt; pSetOpt = pSetOpt->pNext) 
+    {
+        if(strncmp(pSetOpt->pszOptName, TDNF_CONF_KEY_SNAPSHOT_TIME, strlen(TDNF_CONF_KEY_SNAPSHOT_TIME)) == 0)


store strlen(TDNF_CONF_KEY_SNAPSHOT_TIME) in a variable before entering loop

sshedi · 2024-09-26T06:04:33Z

solv/tdnfrepo.c

+        }
+
+    } 
+    else if (pTracking->nInPackage && !pTracking->nTimeFound)


Checking pTracking->nInPackage for non zero is redundant here.

sshedi · 2024-09-26T06:04:51Z

solv/tdnfrepo.c

+            BAIL_ON_TDNF_TIME_FILTER_ERROR(dwError);
+        }
+    }
+    else if (pTracking->nInPackage && !pTracking->nTimeFound)


Checking pTracking->nInPackage for non zero is redundant here.

sshedi · 2024-09-26T07:21:18Z

solv/tdnfrepo.c

+    {
+        BAIL_ON_TDNF_TIME_FILTER_ERROR(dwError);
+    }
+    sprintf(*pszElementBuffer, "</%s>", pszElementName);


use snprintf

sshedi · 2024-09-26T07:40:51Z

solv/tdnfrepo.c

+
+    // allocate new string for linted string
+    int nStrToLintLen = (strlen(pszStringToEscape) + 1); // add one for null char
+    char * pszLintedStr = malloc(nStrToLintLen * sizeof(char));


Use TDNFAllocateString

sshedi · 2024-09-26T08:56:39Z

solv/tdnfrepo.c

+    TDNFFilterData * pTracking = (TDNFFilterData *)userData;
+
+    // decrement depth
+    pTracking->nDepth -= 1;


pTracking->nDepth--; makes i easy to read

sshedi · 2024-09-30T10:29:04Z

solv/tdnfrepo.c

+    {
+        BAIL_ON_TDNF_TIME_FILTER_ERROR(dwError);
+    }
+    fprintf(pbOutfile, "%s", pszEndElement);


have a wrapper function for this, repeated in many places

sshedi · 2024-09-30T10:29:23Z

solv/tdnfrepo.c

+uint32_t 
+printElementEndToFile(FILE * pbOutfile, const char * pszElementName) {
+    uint32_t dwError = 0;
+    if (pbOutfile == NULL)


check using IsNullOrEmptyString

sshedi · 2024-09-30T10:31:49Z

solv/tdnfrepo.c

+        int nEndElementLen = strlen(pszElementBuffer);
+
+        dwError = checkAndResizeBuffer(&(pTracking->pszElementBuffer), &(pTracking->nBufferMaxLen), nEndElementLen);
+        if (dwError)


why this check? BAIL_ON_TDNF_TIME_FILTER_ERROR does it already

sshedi · 2024-09-30T10:32:31Z

solv/tdnfrepo.c

+    // vars
+    uint32_t dwError = 0;
+    TDNFFilterData pData;
+    bzero(&pData, sizeof(TDNFFilterData));


please don't mix declarations and function calls

oliverkurth · 2024-09-30T15:33:21Z

I just did one round of review. Need to understand the logic and usage better.

Thank you @sshedi . But let's not waste our time until @sameluch has answered my questions. I think there are better ways to achieve the same goal.

sameluch · 2024-10-02T18:06:07Z

I just did one round of review. Need to understand the logic and usage better.

Thank you @sshedi . But let's not waste our time until @sameluch has answered my questions. I think there are better ways to achieve the same goal.

Hey @oliverkurth, I'll have some answers coming in soon. I was out of town with little internet access so, but back now to work through this larger change. I will sign the CLA as soon as I am able/allowed.
My apologies for opening a large change request and disappearing haha

@sshedi Thanks for the review I will consider the changes as we decide to move forward here and for future development within the tdnf repo.

sameluch · 2024-10-03T20:28:46Z

Wow, that's a big change. It will take me a while to review this. I have a few question:

What is the use case? Is it just for testing, for example test an upgrade scenario? Or do you want to tag a specific time for a stable build?

Have you considered alternative approaches? For example, wouldn't it be easier to just create a "real" repository with all packages older than the snapshot time (if space is an issue, maybe just use symlinks?). I think this could be done using simple shell scripts, much simpler than this change.

How reliable is the "file" attribute, which is set (I guess) by createrepo? Does it use the actual file time stamp, or current time?

Have you thought about implementing this as a plugin? I know the plugin interface would probably need changes for this. But I think the use case is rather limited.

@oliverkurth here's some answers for your above questions. Let me know if anything is not particularly clear

Use Case
The main focus of this feature is system stability from the client side without much additional overhead of space or calculation. It gives the option to system maintainers and users to "freeze" their system at a point in time for the purposes of stability. Additionally, should updates ever be required to the snapshot, you can simply just update the timestamp in tdnf.conf or use a new time on --snapshottime within a script or otherwise. The goal is client side customization of a repository snapshot without the maintenance overhead of creation/recreation of a manual repo each time.
This could have many uses such as, A-B testing and image recreation as couple cases the top of my head.

Alternate Approaches I've Considered

Adding a similar option/function to libsolv which is currently used to read in repository and for package dependency calculation. Though adding this filtering directly to the package manager as an option seemed much more apt for the actual implementation overhead.
Creation of individual repos for a snapshot given a desired time, this requires much more maintenance by the user or system maintainer. At least for our purposes, this did not seem like the right approach at this time.

The "file" attribute is added by createrepo and is present within each <hash>-primary.xml file. The time stamps used are those from the files, both the "build" and "file" times are collected this way. The "file" time is when the file was added to the repo, the "build" time is when the file in question was actually generated or built. This has been very reliable across remote repos and my local testing.

I did not consider a plugin implementation of this. Do you have an idea of what the changes might look like in that form? I don't quite see how a plugin interface would fit into this solution?
The actual implementation details of the current feature have the enabled, targeted repo files being temporarily copied and altered such that the data being fed to libsolv are those of the effective timestamp being used to leverage the solver in an intuitive way without the requirement of individual repo management.

oliverkurth · 2024-10-03T22:32:03Z

Thanks for replying, @sameluch .

I am not convinced that other approaches are more overhead. Here are some ideas:

creating a separate repo could be simply done with python scripts (or golang if you prefer), which go through the metadata and select just packages with older timestamps. If local, this could be done with symlinks. No impact to tdnf.
after downloading the primary metadata (*-primary.xml.gz), filter it for the timestamps, and replace the original one. This could be done in one of multiple possible ways, for example a plugin, or possibly with a separate script, without impacting tdnf code, or just minimally.
if this needs to be in tdnf itself, instead of rewriting the metadata file as this change does (IIUC), you can use the considered map of the libsolv package pool to filter out packages (looks like you still need to parse the xml separately because apparently the file timestamp is not exposed by libsolv, although the build time is, see SOLVABLE_BUILDTIME in https://github.com/openSUSE/libsolv/blob/master/doc/libsolv-constantids.txt). The considered map is used for example (but not only) for the excludes feature.

The "file" time is when the file was added to the repo,

What does "added" mean? Which time does createrepo use here? Its current time, or the file modification time? Something else? Can you point me to the code in createrepo that does this?

oliverkurth · 2024-10-03T23:27:28Z

It gives the option to system maintainers and users to "freeze" their system at a point in time for the purposes of stability.

What you can do is dump the set of installed packages, to json, like:

root [ / ]# tdnf -j list --installed | jq | head
[
  {
    "Name": "bash",
    "Arch": "aarch64",
    "Evr": "5.2-2.ph5",
    "Repo": "@System"
  },
  {
...

Then use a script, and install on the target with tdnf install bash=5.2-2.ph5 ... etc. So instead of a timestamp, which is pretty unreliable, you use a set of versions.

vmwclabot · 2024-10-07T20:31:22Z

@sameluch, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

Add virtual repo snapshot feature to tdnf

ad4dd2e

sshedi suggested changes Sep 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add virtual repo snapshot feature to tdnf #497

Add virtual repo snapshot feature to tdnf #497

sameluch commented Sep 20, 2024

vmwclabot commented Sep 20, 2024

oliverkurth commented Sep 20, 2024

sshedi left a comment

sshedi Sep 26, 2024

sshedi Sep 26, 2024

sshedi Sep 26, 2024

sshedi Sep 26, 2024

sshedi Sep 26, 2024

sshedi Sep 26, 2024

sshedi Sep 30, 2024

sshedi Sep 30, 2024

sshedi Sep 30, 2024

sshedi Sep 30, 2024

oliverkurth commented Sep 30, 2024

sameluch commented Oct 2, 2024

sameluch commented Oct 3, 2024

oliverkurth commented Oct 3, 2024

oliverkurth commented Oct 3, 2024

vmwclabot commented Oct 7, 2024

Add virtual repo snapshot feature to tdnf #497

Are you sure you want to change the base?

Add virtual repo snapshot feature to tdnf #497

Conversation

sameluch commented Sep 20, 2024

vmwclabot commented Sep 20, 2024

oliverkurth commented Sep 20, 2024

sshedi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverkurth commented Sep 30, 2024

sameluch commented Oct 2, 2024

sameluch commented Oct 3, 2024

oliverkurth commented Oct 3, 2024

oliverkurth commented Oct 3, 2024

vmwclabot commented Oct 7, 2024