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

unawareness runtime prefetch #572

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

billie60
Copy link
Contributor

@billie60 billie60 commented Jan 5, 2024

  1. add prefetchlist store prefetchlist storage service.
  2. Modify the optimizer to publish the access file list as a prefetchlist to the storage service when obtaining it.

@billie60 billie60 force-pushed the unawareness-prefetch branch 3 times, most recently from 059d595 to 2994ad5 Compare January 5, 2024 10:40
@@ -0,0 +1,96 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about naming it tools/prefetch-distribution?

func main() {
serverCache = Cache{Items: make(map[string]*CacheItem)}

http.HandleFunc("/api/v1/prefetch/upload", uploadHandler)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should only accept POST for upload and GET for download, we'd better use the echo web framework.

@@ -104,6 +109,16 @@ func buildFlags(args *PluginArgs) []cli.Flag {
Usage: "whether to overwrite the existed persistent files",
Destination: &args.Config.Overwrite,
},
&cli.StringFlag{
Copy link
Collaborator

Choose a reason for hiding this comment

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

&cli.StringFlag{
  Name:        "prefetch-distribution-endpoint",
  Usage:       "The service endpoint of prefetch distribution, for example: http://localhost:1323/api/v1/prefetch/upload",
  Destination: &args.Config.PrefetchDistributionEndpoint,
},

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any updates?

func (p *plugin) StartContainer(_ *api.PodSandbox, container *api.Container) error {
dir, imageName, err := GetImageName(container.Annotations)
if err != nil {
return err
}
containerName := container.Annotations[containerNameLabel]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the annotation exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, even without adding annotations in container.yaml, it can still use container.Annotations [containerNameLabel] to get container name

Copy link
Member

Choose a reason for hiding this comment

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

If you only need container name, container.Name is enough.

log.WithError(err).Error("failed to send prefetch to http server")
}
}()

globalFanotifyServer[imageName] = fanotifyServer
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should protect the map using mutex.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any updates?


err = postRequest(item, url)
if err != nil {
return fmt.Errorf("error uploading to server: %w", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use errors.Wrap.

func sendToServer(imageName, prefetchlistPath, containerName, serverURL string) error {
data, err := os.ReadFile(prefetchlistPath)
if err != nil {
return fmt.Errorf("error reading file: %w", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use errors.Wrap.

return fmt.Errorf("failed to read response body: %w", err)
}

fmt.Println("Server Response:", string(body))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should check the HTTP status code and return an error if the status code is invalid.

@billie60 billie60 force-pushed the unawareness-prefetch branch 6 times, most recently from bed33a8 to 01d5e46 Compare January 11, 2024 04:53
func (p *plugin) StartContainer(_ *api.PodSandbox, container *api.Container) error {
dir, imageName, err := GetImageName(container.Annotations)
if err != nil {
return err
}
containerName := container.Annotations[containerNameLabel]
Copy link
Member

Choose a reason for hiding this comment

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

If you only need container name, container.Name is enough.

}
}

item := CacheItem{
Copy link
Member

Choose a reason for hiding this comment

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

Can we define the generic API for different storage service? Sometimes, we may need to send more information to the server.

Copy link
Member

Choose a reason for hiding this comment

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

Also, does the API provide a query endpoint?

return c.String(http.StatusBadRequest, "Invalid request payload")
}

serverCache.Set(item)
Copy link
Member

Choose a reason for hiding this comment

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

Can we define an interface for difference storage backend?

Copy link
Member

Choose a reason for hiding this comment

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

I guess can we move the fanotify server to an separate component. Because for containerd 1.6.x, the NRI runs in a different construct. If we do this, the NRI for both containerd versions can be simplified to send a request to server.

@billie60 billie60 changed the title anawareness runtime prefetch unanawareness runtime prefetch Jan 22, 2024
@billie60 billie60 force-pushed the unawareness-prefetch branch 4 times, most recently from d0582fa to b96cf48 Compare January 22, 2024 14:14
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

Attention: Patch coverage is 5.88235% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 34.66%. Comparing base (02b3776) to head (394a502).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #572      +/-   ##
==========================================
+ Coverage   34.64%   34.66%   +0.01%     
==========================================
  Files          65       65              
  Lines        6552     6552              
==========================================
+ Hits         2270     2271       +1     
+ Misses       3967     3966       -1     
  Partials      315      315              
Files Coverage Δ
pkg/daemon/daemon.go 0.00% <ø> (ø)
pkg/system/system.go 6.46% <ø> (+0.30%) ⬆️
config/global.go 19.80% <33.33%> (+0.41%) ⬆️
pkg/manager/daemon_adaptor.go 0.00% <0.00%> (ø)
pkg/manager/manager.go 0.00% <0.00%> (ø)
pkg/daemon/config.go 0.00% <0.00%> (ø)

@billie60 billie60 force-pushed the unawareness-prefetch branch 13 times, most recently from 4111e29 to dcd0f2f Compare January 29, 2024 12:46
@billie60 billie60 force-pushed the unawareness-prefetch branch 7 times, most recently from bf97c9a to 1fd5320 Compare January 30, 2024 07:06
@billie60 billie60 force-pushed the unawareness-prefetch branch 4 times, most recently from 546b4d2 to c2fa245 Compare February 6, 2024 07:58
@imeoer imeoer changed the title unanawareness runtime prefetch unawareness runtime prefetch Feb 27, 2024
sudo systemctl restart nydus-snapshotter
fi
sleep 5
NYDUS_VER=v$(curl --header 'authorization: Bearer ${{ secrets.GITHUB_TOKEN }}' -s "https://api.github.com/repos/dragonflyoss/nydus/releases/latest" | jq -r .tag_name | sed 's/^v//')
Copy link
Collaborator

Choose a reason for hiding this comment

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

The --header 'authorization: Bearer ${{ secrets.GITHUB_TOKEN }}' can be removed.

echo "Error: --prefetch-files not found in nydus-snapshotter.log"
exit 1
fi
# output=$(sudo cat /etc/nydus/logs/nydus-snapshotter.log | grep "nydusd command")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be removed?

@@ -104,6 +109,16 @@ func buildFlags(args *PluginArgs) []cli.Flag {
Usage: "whether to overwrite the existed persistent files",
Destination: &args.Config.Overwrite,
},
&cli.StringFlag{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any updates?

log.WithError(err).Error("failed to send prefetch to http server")
}
}()

globalFanotifyServer[imageName] = fanotifyServer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any updates?

router *mux.Router
}

const endpointImageName string = "/api/v1/imagename"
Copy link
Collaborator

Choose a reason for hiding this comment

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

/api/v1/prefetch?reference=$image

}
defer resp.Body.Close()

responseBody, err := io.ReadAll(resp.Body)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should check if resp.StatusCode != http.StatusOK { before this.

go.mod Outdated
@@ -55,6 +56,12 @@ require (
k8s.io/utils v0.0.0-20230726121419-3b25d923346b
)

require (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Merge these into below.

@@ -0,0 +1,135 @@
version = 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we reuse misc/snapshotter/config.toml instead of creating a new one?

@billie60 billie60 force-pushed the unawareness-prefetch branch 12 times, most recently from dde1a0c to ce605e9 Compare March 4, 2024 10:30
billie60 added 2 commits March 4, 2024 18:28
1. add prefetchlist store prefetchlist storage service.
2. Modify the optimizer to publish the access file list as a prefetchlist to the storage service when obtaining it.

3. modify http server add lru algo
4. use echo web framework
5. modify based on comments

6. get and post prefetchlist in optimizer
1. send post request to optimizer
2. store prefetchlist
3. add prefetchlist in nydusd
@billie60 billie60 force-pushed the unawareness-prefetch branch from ce605e9 to 394a502 Compare March 11, 2024 08:47
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.

4 participants