-
Notifications
You must be signed in to change notification settings - Fork 100
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
Transmit the prefetch list to snapshotter during runp pod.yaml #493
Conversation
@billie60 It seems to have a broken build CI https://github.com/containerd/nydus-snapshotter/actions/runs/5282088960/jobs/9556557174?pr=493 |
cmd/prefetchfiles-nri-plugin/main.go
Outdated
} | ||
|
||
func sendDataOverHTTP(data string, endpoint string) error { | ||
url := "http://172.0.0.1:9110" + endpoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we POST a request to "http://172.0.0.1:9110"? This HTTP URL is used to export metrics and may not always be enabled, so we can not be fully dependent on it. I think the unix domain socket for the system controller is more reasonable.
cmd/prefetchfiles-nri-plugin/main.go
Outdated
type PluginConfig struct { | ||
Events []string `toml:"events"` | ||
ServerPath string `toml:"server_path"` | ||
PersistDir string `toml:"persist_dir"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that this entry is not used in your nri plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it in commits fd34
cmd/prefetchfiles-nri-plugin/main.go
Outdated
return err | ||
} | ||
|
||
conn, err := net.Dial("unix", "/run/containerd-nydus/system.sock") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to set the unix domain socket path from a configuration entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set sockaddr as a command parameter in fd34
cmd/prefetchfiles-nri-plugin/main.go
Outdated
} | ||
|
||
func (p *plugin) onClose() { | ||
for _, fanotifyServer := range globalFanotifyServer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fanotifyServer
is not used by this NRI plugin, please remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed fanotifyServer
cmd/prefetchfiles-nri-plugin/main.go
Outdated
type PluginArgs struct { | ||
PluginName string | ||
PluginIdx string | ||
PluginEvents string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you only need to use RunPodSandbox
interface, you can remove this configuration entry and define it as a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to improve the flexibility of prefetch files transmit, it may be necessary to increase the monitoring of the Create Container event, so I have temporarily kept this part. If any modifications are needed, I will make them the next time I update the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only some specific events can catch the container image name & prefetch list, why do we still need to export the arg?
781e6e6
to
fd34ce9
Compare
cmd/prefetchfiles-nri-plugin/main.go
Outdated
"bytes" | ||
"context" | ||
"fmt" | ||
"github.com/containerd/nydus-snapshotter/pkg/errdefs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The external package should be moved to the bottom.
cmd/prefetchfiles-nri-plugin/main.go
Outdated
const ( | ||
defaultEvents = "RunPodSandbox" | ||
defaultServerPath = "/usr/local/bin/optimizer-server" | ||
defaultPersistDir = "/opt/nri/optimizer/results" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove some irrelevant codes.
cmd/prefetchfiles-nri-plugin/main.go
Outdated
) | ||
|
||
const ( | ||
endpointPL = "/api/v1/daemons/prefetch" //////////todo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: what's todo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endpointPrefetch
cmd/prefetchfiles-nri-plugin/main.go
Outdated
endpointPL = "/api/v1/daemons/prefetch" //////////todo | ||
defaultEvents = "RunPodSandbox" | ||
defaulthttp = "http://system.sock" | ||
defaultsockaddr = "/run/containerd-nydus/system.sock" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaultSockAddr
cmd/prefetchfiles-nri-plugin/main.go
Outdated
type PluginArgs struct { | ||
PluginName string | ||
PluginIdx string | ||
PluginEvents string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only some specific events can catch the container image name & prefetch list, why do we still need to export the arg?
cmd/prefetchfiles-nri-plugin/main.go
Outdated
logWriter *syslog.Writer | ||
) | ||
|
||
func sendDataOverHTTP(data string, endpoint string, sock string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data, endpoint, sock string
cmd/prefetchfiles-nri-plugin/main.go
Outdated
client := &http.Client{ | ||
Transport: &http.Transport{ | ||
DialContext: func(_ context.Context, _, _ string) (net.Conn, error) { | ||
return conn, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should put return net.Dial("unix", sock)
in here?
cmd/prefetchfiles-nri-plugin/main.go
Outdated
|
||
func (p *plugin) RunPodSandbox(pod *api.PodSandbox) error { | ||
name := pod.Name | ||
parts := strings.Split(name, "-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why split the name here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In pod.yaml, the name is defined as image_name-sandbox, splitting the name is to obtain the image_ name. And if the PrefetchList in the States structure needs to be defined as a dictionary in subsequent modifications, image_ name can be used as a key in the dictionary.
cmd/prefetchfiles-nri-plugin/main.go
Outdated
name = parts[0] | ||
if pod.Annotations == nil { | ||
log.Printf("error: Pod annotations is nil") | ||
return errors.New("Pod annotations is nil") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message should start with a lowercase letter.
cmd/prefetchfiles-nri-plugin/main.go
Outdated
|
||
err = p.stub.Run(context.Background()) | ||
if err != nil { | ||
log.Errorf("plugin exited with error %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return errors.Wrap(err, "plugin exited")
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #493 +/- ##
==========================================
- Coverage 37.81% 37.70% -0.12%
==========================================
Files 60 60
Lines 7090 7116 +26
==========================================
+ Hits 2681 2683 +2
- Misses 4097 4121 +24
Partials 312 312
|
9168fc0
to
03307c5
Compare
cmd/prefetchfiles-nri-plugin/main.go
Outdated
} | ||
|
||
var ( | ||
globalsock string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
globalsock
-> globalSocket
cmd/prefetchfiles-nri-plugin/main.go
Outdated
endpointPrefetch = "/api/v1/daemons/prefetch" | ||
defaultEvents = "RunPodSandbox" | ||
defaultHTTPURL = "http://system.sock" | ||
defaultSockaddr = "/run/containerd-nydus/system.sock" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaultSystemControllerAddress
, refer to https://github.com/containerd/nydus-snapshotter/blob/v0.9.0/internal/constant/values.go#L38
cmd/prefetchfiles-nri-plugin/main.go
Outdated
const ( | ||
endpointPrefetch = "/api/v1/daemons/prefetch" | ||
defaultEvents = "RunPodSandbox" | ||
defaultHTTPURL = "http://system.sock" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refer to the implementation that nydus-snapshotter dials nydusd through an unix domain socket. https://github.com/containerd/nydus-snapshotter/blob/v0.9.0/pkg/daemon/client.go#L90
cmd/prefetchfiles-nri-plugin/main.go
Outdated
|
||
func sendDataOverHTTP(data, endpoint, sock string) error { | ||
url := defaultHTTPURL + endpoint | ||
req, err := http.NewRequest("POST", url, bytes.NewBufferString(data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use POST method. Morever we can use a constant http.MethodGet
instead of "GET".
Please refer to https://github.com/containerd/nydus-snapshotter/blob/v0.9.0/pkg/daemon/client.go#L284
cmd/prefetchfiles-nri-plugin/main.go
Outdated
err error | ||
) | ||
|
||
flags.Args.Sockaddr = c.String("sockaddr") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is redundant.
cmd/prefetchfiles-nri-plugin/main.go
Outdated
log.SetFormatter(&logrus.TextFormatter{ | ||
PadLevelText: true, | ||
}) | ||
logWriter, err = syslog.New(syslog.LOG_INFO, "prefetchfiles-nri-plugin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
cmd/prefetchfiles-nri-plugin/main.go
Outdated
type PluginArgs struct { | ||
PluginName string | ||
PluginIdx string | ||
Sockaddr string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SocketAddress
or Address
?
cmd/prefetchfiles-nri-plugin/main.go
Outdated
Destination: &args.PluginIdx, | ||
}, | ||
&cli.StringFlag{ | ||
Name: "sockaddr", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use -
to split two words for command arguments. Just like socket-addr
.
cmd/prefetchfiles-nri-plugin/main.go
Outdated
|
||
p := &plugin{} | ||
|
||
Events := defaultEvents |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is redundant.
aae793e
to
c146069
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks but we still need more work
cmd/prefetchfiles-nri-plugin/main.go
Outdated
) | ||
|
||
const ( | ||
endpointPrefetch = "/api/v1/daemons/prefetch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about replacing the service endpoint with /api/v1/prefetch
since it's possible no nydusd daemon is running while nydus-snapshotter is preparing snapshots
pkg/globalvar/globalvar.go
Outdated
@@ -0,0 +1,10 @@ | |||
package globalvar | |||
|
|||
// var PldataDict = make(map[string]string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid dead code
pkg/daemon/daemon.go
Outdated
@@ -53,6 +53,7 @@ type States struct { | |||
// Where the configuration file resides, all rafs instances share the same configuration template | |||
ConfigDir string | |||
SupervisorPath string | |||
PrefetchFiles string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data in States
field will all be persisted in the DB, is it necessary to store the PrefetchFiles
into DB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, thanks for your review. According to my understanding, adding PrefetchFiles
to States
is to persist this parameter. If PrefetchFiles
is not added to States
, when snapshotter unexpectedly restarts and triggers the recovery process after running the nydusd command, the BuildDaemonCommand
method will discard the --prefetch-files
parameter when rebuilding the nydusd command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this resolved? I suppose it's not. You CAN'T put the prefetch info into DB definitely. I am a little tired of such things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that saving the PrefetchFiles
to DB is not a good choice. You can move it to Rafs
struct if this value is different to each instance. If we need to store it in the DB, please provide more details. TKS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sooo sorry that many of my previous replies to comments were in a pending state, so that you cannot see them. I will reply again, I'm really really sorry.
cmd/prefetchfiles-nri-plugin/main.go
Outdated
if !ok { | ||
errMsg := "pod.yaml annotations don't have prefetch list." | ||
log.Printf("error: %s", errMsg) | ||
return errors.New(errMsg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not return error as it's possible that no prefetch list is ever specified the user
cmd/prefetchfiles-nri-plugin/main.go
Outdated
return errors.New("pod annotations is nil") | ||
} | ||
|
||
prefetchList, ok := pod.Annotations["prefetch_list"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am worrying if the prefetch_list
matches K8s annotation nameing requirement. It should have a name space. Or is it possible to configure the annotation name since different cloud vendors may have different namespaces
cmd/prefetchfiles-nri-plugin/main.go
Outdated
log.Printf("failed to send data: %v\n", err) | ||
return err | ||
} | ||
log.Println("Data sent successfully") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't print tons of normal message to system log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this message.
pkg/system/system.go
Outdated
@@ -167,8 +170,19 @@ func (sc *Controller) registerRouter() { | |||
sc.router.HandleFunc(endpointDaemons, sc.describeDaemons()).Methods(http.MethodGet) | |||
sc.router.HandleFunc(endpointDaemonsUpgrade, sc.upgradeDaemons()).Methods(http.MethodPut) | |||
sc.router.HandleFunc(endpointDaemonRecords, sc.getDaemonRecords()).Methods(http.MethodGet) | |||
sc.router.HandleFunc(endpointPrefetch, sc.getPrefetchFiles()).Methods(http.MethodGet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Http Get method should be used when client wants to query nydus-snapshotter's states. I am suggesting using Put method here
pkg/system/system.go
Outdated
@@ -167,8 +170,19 @@ func (sc *Controller) registerRouter() { | |||
sc.router.HandleFunc(endpointDaemons, sc.describeDaemons()).Methods(http.MethodGet) | |||
sc.router.HandleFunc(endpointDaemonsUpgrade, sc.upgradeDaemons()).Methods(http.MethodPut) | |||
sc.router.HandleFunc(endpointDaemonRecords, sc.getDaemonRecords()).Methods(http.MethodGet) | |||
sc.router.HandleFunc(endpointPrefetch, sc.getPrefetchFiles()).Methods(http.MethodGet) | |||
} | |||
func (sc *Controller) getPrefetchFiles() func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename it setPrefetchFiles ?
pkg/system/system.go
Outdated
func (sc *Controller) getPrefetchFiles() func(w http.ResponseWriter, r *http.Request) { | ||
return func(w http.ResponseWriter, r *http.Request) { | ||
body, _ := io.ReadAll(r.Body) | ||
err := json.Unmarshal(body, &globalvar.Msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deserializing the body to a global variable is not safe. We need to reconsider how to manage the prefetch files list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, other comments have been modified. However, this comment has been tried many ways but no better solution has been found. May I ask if you have any specific suggestions so that I can improve this part of the code again? Thank you very much.
pkg/system/system.go
Outdated
} | ||
func (sc *Controller) getPrefetchFiles() func(w http.ResponseWriter, r *http.Request) { | ||
return func(w http.ResponseWriter, r *http.Request) { | ||
body, _ := io.ReadAll(r.Body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't ignore any retured error
758ab42
to
6dac99b
Compare
cmd/prefetchfiles-nri-plugin/main.go
Outdated
return err | ||
} | ||
|
||
req, err := http.NewRequest(http.MethodPut, url, bytes.NewBuffer(body)) // todo 改为put |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove unnecessary comments.
cmd/prefetchfiles-nri-plugin/main.go
Outdated
return err | ||
} | ||
if resp.StatusCode != http.StatusOK { | ||
return fmt.Errorf("failed to GET request with code %d", resp.StatusCode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"failed to send data, status code: %d"
cmd/prefetchfiles-nri-plugin/main.go
Outdated
return nil | ||
} | ||
|
||
prefetchList, ok := pod.Annotations["containerd.io/snapshotter/nydus-prefetch-list"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about "containerd.io/snapshot/nydus-prefetch"
? In addition, this value should be assigned to a constant.
cmd/prefetchfiles-nri-plugin/main.go
Outdated
prefetchList, ok := pod.Annotations["containerd.io/snapshotter/nydus-prefetch-list"] | ||
if !ok { | ||
errMsg := "pod.yaml annotations don't have prefetch list." | ||
log.Warnf("error: %s", errMsg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to use a variable to hold errMsg
. Moreover, error:
is confused.
I think this message is surplus.
cmd/prefetchfiles-nri-plugin/main.go
Outdated
log.Printf("failed to send data: %v\n", err) | ||
return err | ||
} | ||
log.Println("Data sent successfully") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this message.
pkg/globalvar/globalvar.go
Outdated
package globalvar | ||
|
||
type Message struct { | ||
Name string `json:"name"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the image name
? I think it is not enough to identify a unique image.
pkg/globalvar/globalvar.go
Outdated
|
||
type Message struct { | ||
Name string `json:"name"` | ||
PrefetchFiles string `json:"prefetchList"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure the variables express the same meaning. In addition, we often use underscore naming convention for json data.
pkg/system/system.go
Outdated
return | ||
} | ||
|
||
log.L.Infof("received msg from RunPodSandbox event: %v ", filesystem.PrefetchMsg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
received prefetch list from nri plugin: %v
pkg/system/system.go
Outdated
body, err := io.ReadAll(r.Body) | ||
if err != nil { | ||
log.L.Errorf("Failed to read prefetch list: %v", err) | ||
http.Error(w, "Failed to read prefetch list", http.StatusInternalServerError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return this error to client?
pkg/filesystem/fs.go
Outdated
@@ -45,6 +46,8 @@ const ( | |||
DummyMountpoint string = "/dummy" | |||
) | |||
|
|||
var PrefetchMsg globalvar.Message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be replaced by lru.Cache
. Moreover, we should consider whether global variables should be used.
c22f52c
to
2b1e837
Compare
config/global.go
Outdated
if err := json.Unmarshal(body, &prefetchMsg); err != nil { | ||
return err | ||
} | ||
globalConfig.PrefetchFiles = prefetchMsg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not safe, I suggest to use a map to store this message and use a lock before modifying it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And delete it by key registry/repo/name:tag
when starting nydusd
successfully.
pkg/system/system.go
Outdated
log.L.Errorf("Failed to read prefetch list: %v", err) | ||
return | ||
} | ||
if err = config.SetPrefetchFiles(Msg, body); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the Msg
transmitted to SetPrefetchFiles
is redundant.
Also, please make sure that this plugin doesn't hang for too long if sending data fails, which will result in a Others, LGTM. cc @changweige @imeoer Thanks for @billie60's great work making |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you help add an e2e test for the plugin?
config/global.go
Outdated
PrefetchFiles PrefetchMessage | ||
} | ||
type PrefetchMessage struct { | ||
ImageAddress string `json:"image_address"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename image_address to image_reference which means it format of REPOSITORY:TAG
cmd/prefetchfiles-nri-plugin/main.go
Outdated
defaultEvents = "RunPodSandbox" | ||
defaultSystemControllerAddress = "/run/containerd-nydus/system.sock" | ||
nydusPrefetchAnnotation = "containerd.io/snapshot/nydus-prefetch" | ||
imageAddressAnnotation = "containerd.io/snapshot/nydus-image-address" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this annotation be passed by k8s ? https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/
There are too many slashes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please rename it to nydus-image or nydus-image-reference
config/global.go
Outdated
@@ -37,6 +39,15 @@ type GlobalConfig struct { | |||
DaemonThreadsNum int | |||
CacheGCPeriod time.Duration | |||
MirrorsConfig MirrorsConfig | |||
PrefetchFiles PrefetchMessage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the right place for storing prefetch information
. The struct GlobalConfig
is only for storing configuration items from CLI and toml configuration file
config/global.go
Outdated
if err := json.Unmarshal(body, &prefetchMsg); err != nil { | ||
return err | ||
} | ||
globalConfig.PrefetchFiles = prefetchMsg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, it's not concurrency-safe.
Please refine the commit name. At least, remove the WIP prefix |
38f109b
to
27aee64
Compare
cc @changweige PATL again, thanks! |
Is the commit Merge branch 'main' into prefetchfiles-nri purposed carried on this PR ? Could you squash or fix it? |
038fb4b
to
eebcc0f
Compare
cmd/prefetchfiles-nri-plugin/main.go
Outdated
|
||
log = logrus.StandardLogger() | ||
|
||
viper.SetConfigName("prefetchConfig") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to import the viper
package?
pkg/prefetch/prefetch.go
Outdated
) | ||
|
||
var ( | ||
prefetchMsg MessagePrefetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but I think this global variable may cause some inconsistencies. For instance, if two pods are started, the first one needs to pull a larger image than the second one. Can they get the correct prefetch files? Do you have a test for this case?
pkg/daemon/daemon.go
Outdated
@@ -53,6 +53,7 @@ type States struct { | |||
// Where the configuration file resides, all rafs instances share the same configuration template | |||
ConfigDir string | |||
SupervisorPath string | |||
PrefetchFiles string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that saving the PrefetchFiles
to DB is not a good choice. You can move it to Rafs
struct if this value is different to each instance. If we need to store it in the DB, please provide more details. TKS.
06c3bcd
to
e8ad73c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, looks good
pkg/manager/daemon_adaptor.go
Outdated
@@ -160,6 +165,17 @@ func (m *Manager) BuildDaemonCommand(d *daemon.Daemon, bin string, upgrade bool) | |||
command.WithID(d.ID())) | |||
} | |||
|
|||
prefetchMap := prefetch.GetPrefetchMap() | |||
if imageReference != "" { | |||
prefetchfiles, ok := prefetchMap[imageReference] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessing prefetchMap should be locked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, I have add a lock in GetPrefetchMap
and copy value in it.
cmd/prefetchfiles-nri-plugin/main.go
Outdated
|
||
err := sendDataOverHTTP(prefetchList, endpointPrefetch, globalSocket) | ||
if err != nil { | ||
log.Errorf("failed to send data: %v\n", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to append \n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
cmd/prefetchfiles-nri-plugin/main.go
Outdated
log = logrus.StandardLogger() | ||
|
||
configFileName := "prefetchConfig.toml" | ||
configPath := "." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you man configDir? configPath looks like a path to the config file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though current dir .
looks strange. If NRI plugin process' current dir has the config file, it is not necessary to have it prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
cmd/prefetchfiles-nri-plugin/main.go
Outdated
if err != nil { | ||
log.Warnf("failed to read config file: %v", err) | ||
} | ||
if configSocketAddr := config.Get("file_prefetch.socket_address").(string); configSocketAddr != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type assertion may fail, please handl it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! fixed
Can we have a doc to describe how to deploy the NRI plugin and an associated e2e test would be greater |
a4792cf
to
7584fee
Compare
1. modify config file's default dir in /etc/nydus/ 2. modify GetPrefetchMap in prefetch package to GetPrefetchInfo
7584fee
to
983f342
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM~ Thanks for your contribution! 👍
Hopefully, we can have following PRs:
|
Hello, in this PR, I start an NRI plugin and subscribe to pod creation event. The plugin invokes the RunPodSandbox method to obtain prefetch list file path recorded in pod.yaml when monitoring the pod creation command. Then send the prefetch list file path to snapshotter through the specified socket API. After receiving prefetch list file path, snapshotter will save the content and add --prefetch-files parameter when building nydusd command to customize prefetch files content. I will submit the modifications to snapshotter in the next PR.