> The String parameter to 'nodeLiveFieldExtract' is the query2 field
> name, not the RPC-layer field name. Grrr for not having a real data
> type for this.
Huh, this is interesting - I had it with FieldName originally, but
changed since the string values here were the same as names of the
fields used for dictionary creation (see Rpc.hs) and I hoped this can
then be at some point automated.
On Fri, Oct 05, 2012 at 09:42:34AM +0200, Agata Murawska wrote:
> 2012/10/5 Iustin Pop <ius...@google.com>:
> > The String parameter to 'nodeLiveFieldExtract' is the query2 field
> > name, not the RPC-layer field name. Grrr for not having a real data
> > type for this.
> Huh, this is interesting - I had it with FieldName originally, but
> changed since the string values here were the same as names of the
> fields used for dictionary creation (see Rpc.hs) and I hoped this can
> then be at some point automated.
I think you are mistaken. The bug was exactly that you used the same
string, when the opcode-layer and RPC-layer strings are different.
> > diff --git a/htools/Ganeti/Query/Node.hs b/htools/Ganeti/Query/Node.hs
> > index ded9979..0630754 100644
> > --- a/htools/Ganeti/Query/Node.hs
> > +++ b/htools/Ganeti/Query/Node.hs
> > @@ -72,29 +72,31 @@ nodeLiveFieldsDefs =
> > -- the RPC result.
> > nodeLiveFieldExtract :: String -> RpcResultNodeInfo -> J.JSValue
> I may have gaps in my memory (no code on this machine ;) ), but
> shouldn't the type be FieldName not String now?
Indeed, but type FieldName = String, so I didn't pay too much attention.
Consider it fixed.
On Fri, Oct 05, 2012 at 09:46:48AM +0200, Agata Murawska wrote:
> 2012/10/5 Iustin Pop <ius...@google.com>:
> > The disk free/total values are optional ones, wrapped in a Maybe, so
> > we shouldn't directly serialise them. In order to simplify the
> > embedded extraction, we add a small helper function.
> > +-- | Helper for extracting Maybe values from a possibly empty list.
> > +getMaybeJsonHead :: (J.JSON b) => [a] -> (a -> Maybe b) -> J.JSValue
> > +getMaybeJsonHead [] _ = J.JSNull
> > +getMaybeJsonHead (x:_) f = maybe J.JSNull J.showJSON (f x)
> I think this should be in JSON.hs, just in case we'd ever need that again
> A lot of the lists in Makefile.am were not sorted properly (or at
> all); let's sort them for more sanity.
> Additionally, check-local used to spew this big shell block, even
> though it does emit nice messages when failing, so we don't need to
> show the code; let's silence it (@).
> This is, I believe, the last non-htools specific file that still lived
> in the htools directory; it's already widely used in non-htools code,
> so let's move it before we add more functionality to this module.
> All changes are related to the name change, imports fixup, etc.; there
> are no other changes in this patch.
> Newer GHC refuses to allow "-O" with interactive mode, so let's filter
> that out. Furthermore, sometimes you don't have a clean tree exactly
> when you need to look up something/update the tags, so let's filter
> out the "-Werror" too.
> And finally, we need to pass the actual exact flags (including
> nocurl, parallel, etc.) that we use for building, so let's add those
> too.
On Fri, Oct 05, 2012 at 08:29:41PM +0200, Iustin Pop wrote:
> On Fri, Oct 05, 2012 at 09:46:48AM +0200, Agata Murawska wrote:
> > 2012/10/5 Iustin Pop <ius...@google.com>:
> > > The disk free/total values are optional ones, wrapped in a Maybe, so
> > > we shouldn't directly serialise them. In order to simplify the
> > > embedded extraction, we add a small helper function.
> > > +-- | Helper for extracting Maybe values from a possibly empty list.
> > > +getMaybeJsonHead :: (J.JSON b) => [a] -> (a -> Maybe b) -> J.JSValue
> > > +getMaybeJsonHead [] _ = J.JSNull
> > > +getMaybeJsonHead (x:_) f = maybe J.JSNull J.showJSON (f x)
> > I think this should be in JSON.hs, just in case we'd ever need that again
> Thanks, make sense. Will send interdiff.
And here it is:
diff --git a/htools/Ganeti/JSON.hs b/htools/Ganeti/JSON.hs
index 7c7dd34..178915a 100644
--- a/htools/Ganeti/JSON.hs
+++ b/htools/Ganeti/JSON.hs
@@ -32,6 +32,7 @@ module Ganeti.JSON
, fromKeyValue
, fromJVal
, jsonHead
+ , getMaybeJsonHead
, asJSObject
, asObjectList
, tryFromObj
@@ -131,6 +132,11 @@ jsonHead :: (J.JSON b) => [a] -> (a -> b) -> J.JSValue
jsonHead [] _ = J.JSNull
jsonHead (x:_) f = J.showJSON $ f x
+-- | Helper for extracting Maybe values from a possibly empty list.
+getMaybeJsonHead :: (J.JSON b) => [a] -> (a -> Maybe b) -> J.JSValue
+getMaybeJsonHead [] _ = J.JSNull
+getMaybeJsonHead (x:_) f = maybe J.JSNull J.showJSON (f x)
+
-- | Converts a JSON value into a JSON object.
asJSObject :: (Monad m) => J.JSValue -> m (J.JSObject J.JSValue)
asJSObject (J.JSObject a) = return a
diff --git a/htools/Ganeti/Query/Node.hs b/htools/Ganeti/Query/Node.hs
index 01d0a89..06ec199 100644
--- a/htools/Ganeti/Query/Node.hs
+++ b/htools/Ganeti/Query/Node.hs
@@ -68,11 +68,6 @@ nodeLiveFieldsDefs =
"Total amount of memory of physical machine")
]
--- | Helper for extracting Maybe values from a possibly empty list.
-getMaybeJsonHead :: (J.JSON b) => [a] -> (a -> Maybe b) -> J.JSValue
-getMaybeJsonHead [] _ = J.JSNull
-getMaybeJsonHead (x:_) f = maybe J.JSNull J.showJSON (f x)
-
-- | Map each name to a function that extracts that value from
-- the RPC result.
nodeLiveFieldExtract :: String -> RpcResultNodeInfo -> J.JSValue
> This patch adds a NiceSort equivalent and the corresponding unittest
> (partially copied from Python unittest). The difference between the
> Python version and this one is that this implementation doesn't use
> regular expressions, and as such it doesn't have the 8-groups
> limitation.
> The key-based version is separate from the non-key one (since we don't
> have default arguments in Haskell), and is tested less in its absolute
> properties but only that it is identical to the non-key version under
> some transformations (the non-key version is much more tested).
> This will be needed later in query name sorting.
On Fri, Oct 5, 2012 at 4:17 AM, Iustin Pop <ius...@google.com> wrote:
> We try to automatically enable the htools-rapi and split query (if
> confd and htools-rapi are enabled) options. This is our intended
> default configuration, and allows easier test of the new code
> path. Further cleanups for checking whether confd can be enabled will
> come later.
> The move block is due to the fact that we first have to check for
> htools-rapi, and only then we can auto-enable the feature.
> This makes the "all" names queries consistent with the Python
> results. The change requires updating the unittests, at which point a
> duplicate error message is simplified.
On Wed, Oct 10, 2012 at 11:54:48AM +0200, Michael Hanselmann wrote:
> 2012/10/5 Iustin Pop <ius...@google.com>:
> > --- a/htools/Ganeti/Query/Query.hs
> > +++ b/htools/Ganeti/Query/Query.hs
> > @@ -103,7 +103,9 @@ maybeCollectLiveData False _ nodes =
> > maybeCollectLiveData True cfg nodes = do
> > let vgs = [clusterVolumeGroupName $ configCluster cfg]
> > - hvs = clusterEnabledHypervisors $ configCluster cfg
> > + hvs = case clusterEnabledHypervisors $ configCluster cfg of
> > + [] -> [XenPvm] -- this case shouldn't happen, but we handle it
> Why do you hardcode XenPvm here? Shouldn't you rather raise an
> exception or have this in a global place?
Eee, I was thinking exactly about moving this into a
getDefaultHypervisor function in Config.hs, but then though "if I send
an interdiff, I will delay review even more".
+-- | Returns the default cluster hypervisor.
+getDefaultHypervisor :: ConfigData -> Hypervisor
+getDefaultHypervisor cfg =
+ case clusterEnabledHypervisors $ configCluster cfg of
+ -- FIXME: this case shouldn't happen (configuration broken), but
+ -- for now we handle it here because we're not authoritative for
+ -- the config
+ [] -> XenPvm
+ x:_ -> x
+
-- | Returns instances of a given link.
getInstancesIpByLink :: LinkIpMap -> String -> [String]
getInstancesIpByLink linkipmap link =
diff --git a/htools/Ganeti/Query/Query.hs b/htools/Ganeti/Query/Query.hs
index ff7d33d..7a172ff 100644
--- a/htools/Ganeti/Query/Query.hs
+++ b/htools/Ganeti/Query/Query.hs
@@ -56,6 +56,7 @@ import Data.Maybe (fromMaybe)
import qualified Data.Map as Map
> On Wed, Oct 10, 2012 at 11:54:48AM +0200, Michael Hanselmann wrote:
>> 2012/10/5 Iustin Pop <ius...@google.com>:
>> > --- a/htools/Ganeti/Query/Query.hs
>> > +++ b/htools/Ganeti/Query/Query.hs
>> > @@ -103,7 +103,9 @@ maybeCollectLiveData False _ nodes =
>> > maybeCollectLiveData True cfg nodes = do
>> > let vgs = [clusterVolumeGroupName $ configCluster cfg]
>> > - hvs = clusterEnabledHypervisors $ configCluster cfg
>> > + hvs = case clusterEnabledHypervisors $ configCluster cfg of
>> > + [] -> [XenPvm] -- this case shouldn't happen, but we handle it
>> Why do you hardcode XenPvm here? Shouldn't you rather raise an
>> exception or have this in a global place?
> Eee, I was thinking exactly about moving this into a
> getDefaultHypervisor function in Config.hs, but then though "if I send
> an interdiff, I will delay review even more".
On Fri, Oct 05, 2012 at 08:29:28PM +0200, Iustin Pop wrote:
> On Fri, Oct 05, 2012 at 09:42:34AM +0200, Agata Murawska wrote:
> > 2012/10/5 Iustin Pop <ius...@google.com>:
> > > The String parameter to 'nodeLiveFieldExtract' is the query2 field
> > > name, not the RPC-layer field name. Grrr for not having a real data
> > > type for this.
> > Huh, this is interesting - I had it with FieldName originally, but
> > changed since the string values here were the same as names of the
> > fields used for dictionary creation (see Rpc.hs) and I hoped this can
> > then be at some point automated.
> I think you are mistaken. The bug was exactly that you used the same
> string, when the opcode-layer and RPC-layer strings are different.
> > > Furthermore, we add some safety check that we don't return JSNull via
> > > rsNormal…
-- | Map each name to a function that extracts that value from
-- the RPC result.
-nodeLiveFieldExtract :: String -> RpcResultNodeInfo -> J.JSValue
+nodeLiveFieldExtract :: FieldName -> RpcResultNodeInfo -> J.JSValue
nodeLiveFieldExtract "bootid" res =
J.showJSON $ rpcResNodeInfoBootId res
nodeLiveFieldExtract "cnodes" res =
> On Fri, Oct 05, 2012 at 08:29:28PM +0200, Iustin Pop wrote:
>> On Fri, Oct 05, 2012 at 09:42:34AM +0200, Agata Murawska wrote:
>> > 2012/10/5 Iustin Pop <ius...@google.com>:
>> > > The String parameter to 'nodeLiveFieldExtract' is the query2 field
>> > > name, not the RPC-layer field name. Grrr for not having a real data
>> > > type for this.
>> > Huh, this is interesting - I had it with FieldName originally, but
>> > changed since the string values here were the same as names of the
>> > fields used for dictionary creation (see Rpc.hs) and I hoped this can
>> > then be at some point automated.
>> I think you are mistaken. The bug was exactly that you used the same
>> string, when the opcode-layer and RPC-layer strings are different.
>> > > Furthermore, we add some safety check that we don't return JSNull via
>> > > rsNormal…
> -- | Map each name to a function that extracts that value from
> -- the RPC result.
> -nodeLiveFieldExtract :: String -> RpcResultNodeInfo -> J.JSValue
> +nodeLiveFieldExtract :: FieldName -> RpcResultNodeInfo -> J.JSValue
> nodeLiveFieldExtract "bootid" res =
> J.showJSON $ rpcResNodeInfoBootId res
> nodeLiveFieldExtract "cnodes" res =
On Wed, Oct 10, 2012 at 01:14:04PM +0200, Agata Murawska wrote:
> 2012/10/10 Iustin Pop <ius...@google.com>:
> > On Fri, Oct 05, 2012 at 08:29:28PM +0200, Iustin Pop wrote:
> >> On Fri, Oct 05, 2012 at 09:42:34AM +0200, Agata Murawska wrote:
> >> > 2012/10/5 Iustin Pop <ius...@google.com>:
> >> > > The String parameter to 'nodeLiveFieldExtract' is the query2 field
> >> > > name, not the RPC-layer field name. Grrr for not having a real data
> >> > > type for this.
> >> > Huh, this is interesting - I had it with FieldName originally, but
> >> > changed since the string values here were the same as names of the
> >> > fields used for dictionary creation (see Rpc.hs) and I hoped this can
> >> > then be at some point automated.
> >> I think you are mistaken. The bug was exactly that you used the same
> >> string, when the opcode-layer and RPC-layer strings are different.
> >> > > Furthermore, we add some safety check that we don't return JSNull via
> >> > > rsNormal…
On Mon, Oct 08, 2012 at 01:29:15PM +0200, Iustin Pop wrote:
> On Fri, Oct 05, 2012 at 08:29:41PM +0200, Iustin Pop wrote:
> > On Fri, Oct 05, 2012 at 09:46:48AM +0200, Agata Murawska wrote:
> > > 2012/10/5 Iustin Pop <ius...@google.com>:
> > > > The disk free/total values are optional ones, wrapped in a Maybe, so
> > > > we shouldn't directly serialise them. In order to simplify the
> > > > embedded extraction, we add a small helper function.
> > > > +-- | Helper for extracting Maybe values from a possibly empty list.
> > > > +getMaybeJsonHead :: (J.JSON b) => [a] -> (a -> Maybe b) -> J.JSValue
> > > > +getMaybeJsonHead [] _ = J.JSNull
> > > > +getMaybeJsonHead (x:_) f = maybe J.JSNull J.showJSON (f x)
> > > I think this should be in JSON.hs, just in case we'd ever need that again
> On Mon, Oct 08, 2012 at 01:29:15PM +0200, Iustin Pop wrote:
>> On Fri, Oct 05, 2012 at 08:29:41PM +0200, Iustin Pop wrote:
>> > On Fri, Oct 05, 2012 at 09:46:48AM +0200, Agata Murawska wrote:
>> > > 2012/10/5 Iustin Pop <ius...@google.com>:
>> > > > The disk free/total values are optional ones, wrapped in a Maybe, so
>> > > > we shouldn't directly serialise them. In order to simplify the
>> > > > embedded extraction, we add a small helper function.
>> > > > +-- | Helper for extracting Maybe values from a possibly empty list.
>> > > > +getMaybeJsonHead :: (J.JSON b) => [a] -> (a -> Maybe b) -> J.JSValue
>> > > > +getMaybeJsonHead [] _ = J.JSNull
>> > > > +getMaybeJsonHead (x:_) f = maybe J.JSNull J.showJSON (f x)
>> > > I think this should be in JSON.hs, just in case we'd ever need that again
On Wed, Oct 10, 2012 at 03:34:47PM +0200, Agata Murawska wrote:
> 2012/10/10 Iustin Pop <ius...@google.com>:
> > On Mon, Oct 08, 2012 at 01:29:15PM +0200, Iustin Pop wrote:
> >> On Fri, Oct 05, 2012 at 08:29:41PM +0200, Iustin Pop wrote:
> >> > On Fri, Oct 05, 2012 at 09:46:48AM +0200, Agata Murawska wrote:
> >> > > 2012/10/5 Iustin Pop <ius...@google.com>:
> >> > > > The disk free/total values are optional ones, wrapped in a Maybe, so
> >> > > > we shouldn't directly serialise them. In order to simplify the
> >> > > > embedded extraction, we add a small helper function.
> >> > > > +-- | Helper for extracting Maybe values from a possibly empty list.
> >> > > > +getMaybeJsonHead :: (J.JSON b) => [a] -> (a -> Maybe b) -> J.JSValue
> >> > > > +getMaybeJsonHead [] _ = J.JSNull
> >> > > > +getMaybeJsonHead (x:_) f = maybe J.JSNull J.showJSON (f x)
> >> > > I think this should be in JSON.hs, just in case we'd ever need that again
> This replicates in the Haskell Query2 implementation the behaviour of
> the Python code: if a "simple" filter is passed (one that contains
> only Or aggregators and EQ binary ops on the name field), then an
> failure is flagged if the given values are not known.
> Our implementation is pretty straightforward, with a few details:
> - we ignore any NumericValues passed, since that inconsistency will be
> flagged by the filter compiler
> - we return an the non-normalized names from the getRequestedNames
> function, and not the fully-normalized ones; this will be done later
> in individual query functions
> - we test a few of the desired behaviours of the above-mentioned
> function
> We do this not quite generically, which means we have to add
> another layer in the call chain, and rename the current query
> function, plus add special-case code for each query type. Hopefully we
> will be able to improve on this in the future.
> A (good) side effect of this patch is that we get the desired
> ordering when names are requested, matching the Python code.
> @@ -146,13 +148,23 @@ query :: ConfigData -- ^ The current configuration
> -> Bool -- ^ Whether to collect live data
> -> Query -- ^ The query (item, fields, filter)
> -> IO (Result QueryResult) -- ^ Result
> +query cfg live qry = queryInner cfg live qry $ getRequestedNames qry
> -query cfg live (Query QRNode fields qfilter) = runResultT $ do
> +-- | Inner query execution function.
> +queryInner :: ConfigData -- ^ The current configuration
> + -> Bool -- ^ Whether to collect live data
> + -> Query -- ^ The query (item, fields, filter)
> + -> [String] -- ^ Requested names
> + -> IO (Result QueryResult) -- ^ Result
> +
> +queryInner cfg live (Query QRNode fields qfilter) wanted = runResultT $ do
> cfilter <- resultT $ compileFilter nodeFieldsMap qfilter
> let selected = getSelectedFields nodeFieldsMap fields
> (fdefs, fgetters) = unzip selected
> - nodes = Map.elems . fromContainer $ configNodes cfg
> live' = live && needsLiveData fgetters
> + nodes <- resultT $ case wanted of
> + [] -> Ok . Map.elems . fromContainer $ configNodes cfg
> + _ -> mapM (getNode cfg) wanted
> -- runs first pass of the filter, without a runtime context; this
> -- will limit the nodes that we'll contact for runtime data
> fnodes <- resultT $ filterM (\n -> evaluateFilter cfg Nothing n cfilter) nodes
> @@ -163,21 +175,23 @@ query cfg live (Query QRNode fields qfilter) = runResultT $ do
> nruntimes
> return QueryResult { qresFields = fdefs, qresData = fdata }
> -query cfg _ (Query QRGroup fields qfilter) = return $ do
> +queryInner cfg _ (Query QRGroup fields qfilter) wanted = return $ do
> -- FIXME: want_diskparams is defaulted to false and not taken as parameter
> -- This is because the type for DiskParams is right now too generic for merges
> -- (or else I cannot see how to do this with curent implementation)
> cfilter <- compileFilter groupFieldsMap qfilter
> let selected = getSelectedFields groupFieldsMap fields
> (fdefs, fgetters) = unzip selected
> - groups = Map.elems . fromContainer $ configNodegroups cfg
> + groups <- case wanted of
> + [] -> Ok . Map.elems . fromContainer $ configNodegroups cfg
> + _ -> mapM (getGroup cfg) wanted
> -- there is no live data for groups, so filtering is much simpler
> fgroups <- filterM (\n -> evaluateFilter cfg Nothing n cfilter) groups
> let fdata = map (\node ->
> map (execGetter cfg GroupRuntime node) fgetters) fgroups
> return QueryResult {qresFields = fdefs, qresData = fdata }
